[Password Manager] Fix to recognise failed login attempt for sites where...

[Password Manager] Fix to recognise failed login attempt for sites where content server pushes new login form.

With current implementation chromium browser does not recognize the failed login attempt for sites where content server pushes different login form (e.g. http://www.xda-developers.com), and apparently it assumes a login success and offers to save an incorrect password.

With this patch in addition to submitted form's action URL to that of visible form's action URL it ignores the schemes for HTTP or HTTPS URLs as well.

BUG=400769

Review URL: https://codereview.chromium.org/488083002

Cr-Commit-Position: refs/heads/master@{#292877}

chrome/browser/password_manager/password_manager_browsertest.cc

@@ -21,6 +21,7 @@
 #include "chrome/browser/ui/login/login_prompt_test_utils.h"
 #include "chrome/browser/ui/passwords/manage_passwords_ui_controller.h"
 #include "chrome/browser/ui/tabs/tab_strip_model.h"
+#include "chrome/common/chrome_switches.h"
 #include "chrome/common/chrome_version_info.h"
 #include "chrome/test/base/in_process_browser_test.h"
 #include "chrome/test/base/test_switches.h"
@@ -37,11 +38,13 @@
 #include "content/public/browser/render_view_host.h"
 #include "content/public/browser/web_contents.h"
 #include "content/public/browser/web_contents_observer.h"
+#include "content/public/common/content_switches.h"
 #include "content/public/test/browser_test_utils.h"
 #include "content/public/test/test_utils.h"
 #include "net/test/embedded_test_server/embedded_test_server.h"
 #include "net/test/embedded_test_server/http_request.h"
 #include "net/test/embedded_test_server/http_response.h"
+#include "net/test/spawned_test_server/spawned_test_server.h"
 #include "net/url_request/test_url_fetcher_factory.h"
 #include "testing/gmock/include/gmock/gmock.h"
 #include "third_party/WebKit/public/web/WebInputEvent.h"
@@ -1119,3 +1122,59 @@ IN_PROC_BROWSER_TEST_F(PasswordManagerBrowserTest,
   EXPECT_TRUE(prompt_observer->IsShowingPrompt());
 }
 
+// Test that if login fails and content server pushes a different login form
+// with action URL having different schemes. Heuristic shall be able
+// identify such cases and *shall not* prompt to save incorrect password.
+IN_PROC_BROWSER_TEST_F(
+    PasswordManagerBrowserTest,
+    NoPromptForLoginFailedAndServerPushSeperateLoginForm_HttpToHttps) {
+  std::string path =
+      "/password/separate_login_form_with_onload_submit_script.html";
+  GURL http_url(embedded_test_server()->GetURL(path));
+  ASSERT_TRUE(http_url.SchemeIs(url::kHttpScheme));
+
+  NavigationObserver observer(WebContents());
+  scoped_ptr<PromptObserver> prompt_observer(
+      PromptObserver::Create(WebContents()));
+  ui_test_utils::NavigateToURL(browser(), http_url);
+
+  observer.SetPathToWaitFor("/password/done_and_separate_login_form.html");
+  observer.Wait();
+
+  EXPECT_FALSE(prompt_observer->IsShowingPrompt());
+}
+
+IN_PROC_BROWSER_TEST_F(
+    PasswordManagerBrowserTest,
+    NoPromptForLoginFailedAndServerPushSeperateLoginForm_HttpsToHttp) {
+  CommandLine::ForCurrentProcess()->AppendSwitch(
+      switches::kAllowRunningInsecureContent);
+  CommandLine::ForCurrentProcess()->AppendSwitch(
+      switches::kIgnoreCertificateErrors);
+  const base::FilePath::CharType kDocRoot[] =
+      FILE_PATH_LITERAL("chrome/test/data");
+  net::SpawnedTestServer https_test_server(
+      net::SpawnedTestServer::TYPE_HTTPS,
+      net::SpawnedTestServer::SSLOptions(
+          net::SpawnedTestServer::SSLOptions::CERT_OK),
+      base::FilePath(kDocRoot));
+  ASSERT_TRUE(https_test_server.Start());
+
+  // This test case cannot inject the scripts via content::ExecuteScript() in
+  // files served through HTTPS. Therefore the scripts are made part of the HTML
+  // site and executed on load.
+  std::string path =
+      "password/separate_login_form_with_onload_submit_script.html";
+  GURL https_url(https_test_server.GetURL(path));
+  ASSERT_TRUE(https_url.SchemeIs(url::kHttpsScheme));
+
+  NavigationObserver observer(WebContents());
+  scoped_ptr<PromptObserver> prompt_observer(
+      PromptObserver::Create(WebContents()));
+  ui_test_utils::NavigateToURL(browser(), https_url);
+
+  observer.SetPathToWaitFor("/password/done_and_separate_login_form.html");
+  observer.Wait();
+
+  EXPECT_FALSE(prompt_observer->IsShowingPrompt());
+}

chrome/test/data/password/done_and_separate_login_form.html

+<html>
+<head>
+<script type="text/javascript">
+window.onload = function() {
+  document.getElementById('to_separate').action = getLocationWithHttpSchemeSecuritySwapped();
+}
+
+function getLocationWithHttpSchemeSecuritySwapped() {
+  var protocol = ("http:" == window.location.protocol) ? "https://" : "http://";
+  var url = protocol + window.location.host + window.location.pathname;
+  return url;
+}
+</script>
+</head>
+<body>
+Navigation complete. Below is the different login form pushed by server but with same action URL.
+The URL schem may or may not be same. Moreover, browser shall not promp user to save incorrect password.
+<form method="POST" id="to_separate">
+  <input type="text" id="username_separate" name="username_separate">
+  <input type="password" id="password_separate" name="password_separate">
+  <input type="submit" id="submit_separate" name="submit_separate">
+</form>
+</body>
+</html>
\ No newline at end of file

chrome/test/data/password/separate_login_form_with_onload_submit_script.html

+<html>
+<head>
+<script type="text/javascript">
+window.onload = function() {
+  document.getElementById('username_separate').value = "user";
+  document.getElementById('password_separate').value = "password";
+  document.getElementById('submit_separate').click();
+}
+</script>
+<head>
+<body>
+<form method="POST" action="/password/done_and_separate_login_form.html" id="to_separate">
+  <input type="text" id="username_separate" name="username_separate">
+  <input type="password" id="password_separate" name="password_separate">
+  <input type="submit" id="submit_separate" name="submit_separate">
+</form>
+</body>
+</html>
\ No newline at end of file

components/password_manager/core/browser/password_manager.cc

@@ -69,6 +69,22 @@ bool ShouldDropSyncCredential() {
   return group_name != "Disabled";
 }
 
+bool URLsEqualUpToScheme(const GURL& a, const GURL& b) {
+  return (a.GetContent() == b.GetContent());
+}
+
+bool URLsEqualUpToHttpHttpsSubstitution(const GURL& a, const GURL& b) {
+  if (a == b)
+    return true;
+
+  // The first-time and retry login forms action URLs sometimes differ in
+  // switching from HTTP to HTTPS, see http://crbug.com/400769.
+  if (a.SchemeIsHTTPOrHTTPS() && b.SchemeIsHTTPOrHTTPS())
+    return URLsEqualUpToScheme(a, b);
+
+  return false;
+}
+
 }  // namespace
 
 const char PasswordManager::kOtherPossibleUsernamesExperiment[] =
@@ -437,12 +453,14 @@ void PasswordManager::OnPasswordFormsRendered(
   // If we see the login form again, then the login failed.
   if (did_stop_loading) {
     for (size_t i = 0; i < all_visible_forms_.size(); ++i) {
-      // TODO(vabr): The similarity check is just action equality for now. If it
-      // becomes more complex, it may make sense to consider modifying and using
+      // TODO(vabr): The similarity check is just action equality up to
+      // HTTP<->HTTPS substitution for now. If it becomes more complex, it may
+      // make sense to consider modifying and using
       // PasswordFormManager::DoesManage for it.
       if (all_visible_forms_[i].action.is_valid() &&
-          provisional_save_manager_->pending_credentials().action ==
-              all_visible_forms_[i].action) {
+          URLsEqualUpToHttpHttpsSubstitution(
+              provisional_save_manager_->pending_credentials().action,
+              all_visible_forms_[i].action)) {
         if (logger) {
           logger->LogPasswordForm(Logger::STRING_PASSWORD_FORM_REAPPEARED,
                                   visible_forms[i]);

components/password_manager/core/browser/password_manager_unittest.cc

@@ -758,4 +758,43 @@ TEST_F(PasswordManagerTest, SyncCredentialsNotSaved) {
   manager()->OnPasswordFormsRendered(observed, true);
 }
 
+// On failed login attempts, the retry-form can have action scheme changed from
+// HTTP to HTTPS (see http://crbug.com/400769). Check that such retry-form is
+// considered equal to the original login form, and the attempt recognised as a
+// failure.
+TEST_F(PasswordManagerTest,
+       SeeingFormActionWithOnlyHttpHttpsChangeIsLoginFailure) {
+  std::vector<PasswordForm*> result;  // Empty password store.
+  EXPECT_CALL(driver_, FillPasswordForm(_)).Times(Exactly(0));
+  EXPECT_CALL(*store_.get(), GetLogins(_, _, _))
+      .WillRepeatedly(DoAll(WithArg<2>(InvokeConsumer(result)), Return()));
+
+  PasswordForm first_form(MakeSimpleForm());
+  first_form.origin = GURL("http://www.xda-developers.com/");
+  first_form.action = GURL("http://forum.xda-developers.com/login.php");
+
+  // |second_form|'s action differs only with it's scheme i.e. *https://*.
+  PasswordForm second_form(first_form);
+  second_form.action = GURL("https://forum.xda-developers.com/login.php");
+
+  std::vector<PasswordForm> observed;
+  observed.push_back(first_form);
+  manager()->OnPasswordFormsParsed(observed);
+  manager()->OnPasswordFormsRendered(observed, true);
+  observed.clear();
+
+  // Now submit the |first_form|.
+  OnPasswordFormSubmitted(first_form);
+
+  // Simulate loading a page, which contains |second_form| instead of
+  // |first_form|.
+  observed.push_back(second_form);
+
+  // Verify that no prompt to save the password is shown.
+  EXPECT_CALL(client_, PromptUserToSavePasswordPtr(_)).Times(Exactly(0));
+  manager()->OnPasswordFormsParsed(observed);
+  manager()->OnPasswordFormsRendered(observed, true);
+  observed.clear();
+}
+
 }  // namespace password_manager