[Passwords] Fix DCHECK in GetSignonRealmWithProtocolExcluded

This change fixes a DCHECK in GetSignonRealmWithProtocolExcluded
triggered when operating on credentials where the form's origin contained
a port, but the signon_realm did not. For example. this can happen for
federated credentials.

Furthermore, this change improves the robustness of InvalidRealmCredentialCleaner
and HttpCredentialCleaner by performing the HTTP / HTTPS check on the form's
signon_realm instead of the origin.

Bug: 871140
Change-Id: If4c531e0c8741db05f5e1b7565c5fd6026836ae6
Reviewed-on: https://chromium-review.googlesource.com/c/1345512
Commit-Queue: Jan Wilken Dörrie <jdoerrie@chromium.org>
Reviewed-by: Vaclav Brozek <vabr@chromium.org>
Cr-Commit-Position: refs/heads/master@{#610120}

components/password_manager/core/browser/BUILD.gn

@@ -57,6 +57,7 @@ jumbo_static_library("browser") {
     "credential_manager_pending_prevent_silent_access_task.h",
     "credential_manager_pending_request_task.cc",
     "credential_manager_pending_request_task.h",
+    "credentials_cleaner.cc",
     "credentials_cleaner.h",
     "credentials_cleaner_runner.cc",
     "credentials_cleaner_runner.h",
@@ -402,6 +403,7 @@ source_set("unit_tests") {
     "credential_manager_logger_unittest.cc",
     "credential_manager_password_form_manager_unittest.cc",
     "credentials_cleaner_runner_unittest.cc",
+    "credentials_cleaner_unittest.cc",
     "export/csv_writer_unittest.cc",
     "export/password_csv_writer_unittest.cc",
     "export/password_manager_exporter_unittest.cc",

components/password_manager/core/browser/credentials_cleaner.cc

+// Copyright 2018 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include "components/password_manager/core/browser/credentials_cleaner.h"
+
+#include "base/stl_util.h"
+#include "components/autofill/core/common/password_form.h"
+#include "url/gurl.h"
+
+namespace password_manager {
+
+// static
+std::vector<std::unique_ptr<autofill::PasswordForm>>
+CredentialsCleaner::RemoveNonHTTPOrHTTPSForms(
+    std::vector<std::unique_ptr<autofill::PasswordForm>> forms) {
+  base::EraseIf(forms, [](const auto& form) {
+    return !GURL(form->signon_realm).SchemeIsHTTPOrHTTPS();
+  });
+
+  return forms;
+}
+
+}  // namespace password_manager

components/password_manager/core/browser/credentials_cleaner.h

@@ -5,6 +5,13 @@
 #ifndef COMPONENTS_PASSWORD_MANAGER_CORE_BROWSER_CREDENTIALS_CLEANER_H_
 #define COMPONENTS_PASSWORD_MANAGER_CORE_BROWSER_CREDENTIALS_CLEANER_H_
 
+#include <memory>
+#include <vector>
+
+namespace autofill {
+struct PasswordForm;
+}
+
 namespace password_manager {
 
 // Interface that allows CredentialsCleanerRunner class to easily manipulate
@@ -35,8 +42,15 @@ class CredentialsCleaner {
   // notified about the completion of the clean-up, so the |observer| should not
   // be null.
   virtual void StartCleaning(Observer* observer) = 0;
+
+  // Iterates through |forms| and removes credentials whose signon_realm does
+  // not correspond to a HTTP or HTTPS scheme. In particular, this filters out
+  // Android and federated credentials. Returns the result.
+  static std::vector<std::unique_ptr<autofill::PasswordForm>>
+  RemoveNonHTTPOrHTTPSForms(
+      std::vector<std::unique_ptr<autofill::PasswordForm>> forms);
 };
 
 }  // namespace password_manager
 
-#endif  // COMPONENTS_PASSWORD_MANAGER_CORE_BROWSER_CREDENTIALS_CLEANER_H_
+#endif  // COMPONENTS_PASSWORD_MANAGER_CORE_BROWSER_CREDENTIALS_CLEANER_H_
\ No newline at end of file

components/password_manager/core/browser/credentials_cleaner_unittest.cc

+// Copyright 2018 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include "components/password_manager/core/browser/credentials_cleaner.h"
+
+#include <string>
+#include <utility>
+
+#include "components/autofill/core/common/password_form.h"
+#include "testing/gmock/include/gmock/gmock.h"
+#include "testing/gtest/include/gtest/gtest.h"
+#include "url/gurl.h"
+
+using autofill::PasswordForm;
+using ::testing::ElementsAre;
+using ::testing::Pointee;
+
+namespace password_manager {
+
+TEST(CredentialsCleaner, RemoveNonHTTPOrHTTPSForms) {
+  std::vector<std::unique_ptr<PasswordForm>> forms;
+
+  PasswordForm http_form;
+  http_form.signon_realm = "http://example.com";
+  forms.push_back(std::make_unique<PasswordForm>(http_form));
+
+  PasswordForm https_form;
+  https_form.signon_realm = "https://example.com";
+  forms.push_back(std::make_unique<PasswordForm>(https_form));
+
+  PasswordForm federated_form;
+  federated_form.signon_realm = "federation://example.com/google.com";
+  forms.push_back(std::make_unique<PasswordForm>(federated_form));
+
+  PasswordForm android_form;
+  android_form.signon_realm = "android://hash@com.example.beta.android";
+  forms.push_back(std::make_unique<PasswordForm>(android_form));
+
+  PasswordForm basic_auth_form;
+  basic_auth_form.signon_realm =
+      "https://example.com/`My` |Realm| %With% <Special> {Chars}\r\t\n\x01";
+  forms.push_back(std::make_unique<PasswordForm>(basic_auth_form));
+  EXPECT_TRUE(GURL(basic_auth_form.signon_realm).is_valid());
+
+  // Note: We are using std::string_literals to be able to construct a
+  // std::string with an embedded null character.
+  using std::string_literals::operator""s;
+  PasswordForm invalid_basic_auth_form;
+  invalid_basic_auth_form.signon_realm = "http://example.com/Invalid\0Realm"s;
+  forms.push_back(std::make_unique<PasswordForm>(invalid_basic_auth_form));
+  EXPECT_FALSE(GURL(invalid_basic_auth_form.signon_realm).is_valid());
+
+  PasswordForm another_invalid_form;
+  another_invalid_form.signon_realm = "http://";
+  forms.push_back(std::make_unique<PasswordForm>(another_invalid_form));
+  EXPECT_FALSE(GURL(another_invalid_form.signon_realm).is_valid());
+
+  // Expect that only the federated and Android form got removed.
+  EXPECT_THAT(
+      CredentialsCleaner::RemoveNonHTTPOrHTTPSForms(std::move(forms)),
+      ElementsAre(Pointee(http_form), Pointee(https_form),
+                  Pointee(basic_auth_form), Pointee(invalid_basic_auth_form),
+                  Pointee(another_invalid_form)));
+}
+
+}  // namespace password_manager

components/password_manager/core/browser/http_credentials_cleaner.cc

@@ -39,12 +39,9 @@ void HttpCredentialCleaner::StartCleaning(Observer* observer) {
 
 void HttpCredentialCleaner::OnGetPasswordStoreResults(
     std::vector<std::unique_ptr<autofill::PasswordForm>> results) {
-  // Non HTTP or HTTPS credentials are ignored.
-  base::EraseIf(results, [](const auto& form) {
-    return !form->origin.SchemeIsHTTPOrHTTPS();
-  });
-
-  for (auto& form : results) {
+  // Non HTTP or HTTPS credentials are ignored, in particular Android or
+  // federated credentials.
+  for (auto& form : RemoveNonHTTPOrHTTPSForms(std::move(results))) {
     FormKey form_key(
         {std::string(
              password_manager_util::GetSignonRealmWithProtocolExcluded(*form)),

components/password_manager/core/browser/invalid_realm_credential_cleaner.cc

@@ -184,15 +184,10 @@ void InvalidRealmCredentialCleaner::OnGetPasswordStoreResults(
     FormVector results) {
   DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
 
-  // Non HTTP or HTTPS credentials are ignored.
-  base::EraseIf(results, [](const auto& form) {
-    return !form->origin.SchemeIsHTTPOrHTTPS();
-  });
-
   // Separate HTTP and HTTPS credentials.
   FormVector http_forms, https_forms;
   std::tie(http_forms, https_forms) = SplitFormsBy(
-      std::move(results),
+      RemoveNonHTTPOrHTTPSForms(std::move(results)),
       [](const auto& form) { return form->origin.SchemeIs(url::kHttpScheme); });
 
   // Map from (date_created, origin (excluding protocol), username_value) of
@@ -237,4 +232,4 @@ void InvalidRealmCredentialCleaner::CleaningFinished() {
   observer_->CleaningCompleted();
 }
 
-}  // namespace password_manager
+}  // namespace password_manager
\ No newline at end of file

components/password_manager/core/browser/password_manager_util.cc

@@ -227,7 +227,7 @@ base::StringPiece GetSignonRealmWithProtocolExcluded(const PasswordForm& form) {
 
   // Find the web origin (with protocol excluded) in the signon_realm.
   const size_t after_protocol =
-      signon_realm_protocol_excluded.find(form.origin.GetOrigin().GetContent());
+      signon_realm_protocol_excluded.find(form.origin.host_piece());
   DCHECK_NE(after_protocol, base::StringPiece::npos);
 
   // Keep the string starting with position |after_protocol|.

components/password_manager/core/browser/password_manager_util.h

@@ -105,7 +105,7 @@ void RemoveUselessCredentials(
 // what is before the web origin (with the protocol excluded as well). For
 // example if the signon_realm is "https://www.google.com/", after
 // excluding protocol it becomes "www.google.com/".
-// This assumes that the |form|'s origin is a substring of the signon_realm.
+// This assumes that the |form|'s host is a substring of the signon_realm.
 base::StringPiece GetSignonRealmWithProtocolExcluded(
     const autofill::PasswordForm& form);
 

components/password_manager/core/browser/password_manager_util_unittest.cc

@@ -219,6 +219,13 @@ TEST(PasswordManagerUtil, GetSignonRealmWithProtocolExcluded) {
   https_form.origin = GURL("https://www.google.com/page-1/");
   https_form.signon_realm = "https://www.google.com/";
   EXPECT_EQ(GetSignonRealmWithProtocolExcluded(https_form), "www.google.com/");
+
+  autofill::PasswordForm federated_form;
+  federated_form.origin = GURL("http://localhost:8000/");
+  federated_form.signon_realm =
+      "federation://localhost/accounts.federation.com";
+  EXPECT_EQ(GetSignonRealmWithProtocolExcluded(federated_form),
+            "localhost/accounts.federation.com");
 }
 
 TEST(PasswordManagerUtil, FindBestMatches) {