[PasswordManager] Disable filling of password suggestions into clear-text fields

Clear-text form fields can be marked as type |NEW_PASSWORD|
by a server-prediction to enable the password-generation feature.
If a field of type |NEW_PASSWORD| is clicked, Chrome not only
offers password generation, but also offers to fill the field with
stored credentials as a fallback option to handle type misclassifications.
For a clear-text field, this fallback mechanism must be disabled to
prevent the user from exposing his credentials.

Bug: 1012234
Change-Id: I9bbc19d7bb22c80c22f0a3782cab6b9f97d57891
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1847279Reviewed-by: Vasilii Sukhanov <vasilii@chromium.org>
Reviewed-by: Mike West <mkwst@chromium.org>
Commit-Queue: Matthias Körber <koerber@google.com>
Cr-Commit-Position: refs/heads/master@{#707281}

chrome/browser/password_manager/chrome_password_manager_client.cc

@@ -22,7 +22,6 @@
 #include "chrome/browser/metrics/chrome_metrics_service_accessor.h"
 #include "chrome/browser/password_manager/account_storage/account_password_store_factory.h"
 #include "chrome/browser/password_manager/password_store_factory.h"
-#include "chrome/browser/password_manager/touch_to_fill_controller.h"
 #include "chrome/browser/prerender/prerender_contents.h"
 #include "chrome/browser/profiles/profile.h"
 #include "chrome/browser/safe_browsing/chrome_password_protection_service.h"
@@ -865,8 +864,9 @@ void ChromePasswordManagerClient::ShowPasswordEditingPopup(
           password_generation_driver_bindings_.GetCurrentTargetFrame(),
           bounds));
   autofill::password_generation::PasswordGenerationUIData ui_data(
-      bounds, 0 /*max_length*/, base::string16() /*generation_element*/,
-      field_renderer_id, base::i18n::TextDirection(), form);
+      bounds, /*max_length=*/0, /*generation_element=*/base::string16(),
+      field_renderer_id, /*is_generation_element_password_type=*/true,
+      base::i18n::TextDirection(), form);
   popup_controller_ = PasswordGenerationPopupControllerImpl::GetOrCreate(
       popup_controller_, element_bounds_in_screen_space, ui_data,
       driver->AsWeakPtr(), observer_, web_contents(),
@@ -1171,10 +1171,13 @@ void ChromePasswordManagerClient::ShowPasswordGenerationPopup(
     bool is_manually_triggered) {
   gfx::RectF element_bounds_in_top_frame_space =
       TransformToRootCoordinates(driver->render_frame_host(), ui_data.bounds);
+  // Only show password suggestions iff the field is of password type.
+  bool show_password_suggestions = ui_data.is_generation_element_password_type;
   if (!is_manually_triggered &&
       driver->GetPasswordAutofillManager()
           ->MaybeShowPasswordSuggestionsWithGeneration(
-              element_bounds_in_top_frame_space, ui_data.text_direction)) {
+              element_bounds_in_top_frame_space, ui_data.text_direction,
+              show_password_suggestions)) {
     return;
   }
 

chrome/browser/ui/passwords/password_generation_popup_controller_impl_unittest.cc

@@ -4,6 +4,8 @@
 
 #include "chrome/browser/ui/passwords/password_generation_popup_controller_impl.h"
 
+#include <memory>
+
 #include "base/strings/utf_string_conversions.h"
 #include "chrome/test/base/chrome_render_view_host_test_harness.h"
 #include "components/autofill/core/common/password_form.h"
@@ -41,8 +43,9 @@ PasswordGenerationPopupControllerImplTest::CreateDriver() {
 
 TEST_F(PasswordGenerationPopupControllerImplTest, GetOrCreateTheSame) {
   autofill::password_generation::PasswordGenerationUIData ui_data(
-      gfx::RectF(100, 20), 20 /*max_length*/, base::ASCIIToUTF16("element"),
-      100 /*generation_element_id*/, base::i18n::TextDirection(),
+      gfx::RectF(100, 20), /*max_length=*/20, base::ASCIIToUTF16("element"),
+      /*generation_element_id=*/100,
+      /*is_generation_element_password_type=*/true, base::i18n::TextDirection(),
       autofill::PasswordForm());
   ui_data.password_form.username_value = base::ASCIIToUTF16("Name");
   ui_data.password_form.password_value = base::ASCIIToUTF16("12345");
@@ -64,8 +67,9 @@ TEST_F(PasswordGenerationPopupControllerImplTest, GetOrCreateTheSame) {
 TEST_F(PasswordGenerationPopupControllerImplTest, GetOrCreateDifferentBounds) {
   gfx::RectF rect(100, 20);
   autofill::password_generation::PasswordGenerationUIData ui_data(
-      rect, 20 /*max_length*/, base::ASCIIToUTF16("element"),
-      100 /*generation_element_id*/, base::i18n::TextDirection(),
+      rect, /*max_length=*/20, base::ASCIIToUTF16("element"),
+      /*generation_element_id=*/100,
+      /*is_generation_element_password_type=*/true, base::i18n::TextDirection(),
       autofill::PasswordForm());
   ui_data.password_form.username_value = base::ASCIIToUTF16("Name");
   ui_data.password_form.password_value = base::ASCIIToUTF16("12345");
@@ -88,8 +92,9 @@ TEST_F(PasswordGenerationPopupControllerImplTest, GetOrCreateDifferentBounds) {
 
 TEST_F(PasswordGenerationPopupControllerImplTest, GetOrCreateDifferentTabs) {
   autofill::password_generation::PasswordGenerationUIData ui_data(
-      gfx::RectF(100, 20), 20 /*max_length*/, base::ASCIIToUTF16("element"),
-      100 /*generation_element_id*/, base::i18n::TextDirection(),
+      gfx::RectF(100, 20), /*max_length=*/20, base::ASCIIToUTF16("element"),
+      /*generation_element_id=*/100,
+      /*is_generation_element_password_type=*/true, base::i18n::TextDirection(),
       autofill::PasswordForm());
   ui_data.password_form.username_value = base::ASCIIToUTF16("Name");
   ui_data.password_form.password_value = base::ASCIIToUTF16("12345");
@@ -112,8 +117,9 @@ TEST_F(PasswordGenerationPopupControllerImplTest, GetOrCreateDifferentTabs) {
 
 TEST_F(PasswordGenerationPopupControllerImplTest, GetOrCreateDifferentDrivers) {
   autofill::password_generation::PasswordGenerationUIData ui_data(
-      gfx::RectF(100, 20), 20 /*max_length*/, base::ASCIIToUTF16("element"),
-      100 /*generation_element_id*/, base::i18n::TextDirection(),
+      gfx::RectF(100, 20), /*max_length=*/20, base::ASCIIToUTF16("element"),
+      /*generation_element_id=*/100,
+      /*is_generation_element_password_type=*/true, base::i18n::TextDirection(),
       autofill::PasswordForm());
   ui_data.password_form.username_value = base::ASCIIToUTF16("Name");
   ui_data.password_form.password_value = base::ASCIIToUTF16("12345");
@@ -137,8 +143,9 @@ TEST_F(PasswordGenerationPopupControllerImplTest, GetOrCreateDifferentDrivers) {
 TEST_F(PasswordGenerationPopupControllerImplTest,
        GetOrCreateDifferentElements) {
   autofill::password_generation::PasswordGenerationUIData ui_data(
-      gfx::RectF(100, 20), 20 /*max_length*/, base::ASCIIToUTF16("element"),
-      100 /*generation_element_id*/, base::i18n::TextDirection(),
+      gfx::RectF(100, 20), /*max_length=*/20, base::ASCIIToUTF16("element"),
+      /*generation_element_id=*/100,
+      /*is_generation_element_password_type=*/true, base::i18n::TextDirection(),
       autofill::PasswordForm());
   auto driver = CreateDriver();
   std::unique_ptr<content::WebContents> web_contents = CreateTestWebContents();
@@ -159,8 +166,9 @@ TEST_F(PasswordGenerationPopupControllerImplTest,
 
 TEST_F(PasswordGenerationPopupControllerImplTest, DestroyInPasswordAccepted) {
   autofill::password_generation::PasswordGenerationUIData ui_data(
-      gfx::RectF(100, 20), 20 /*max_length*/, base::ASCIIToUTF16("element"),
-      100 /*generation_element_id*/, base::i18n::TextDirection(),
+      gfx::RectF(100, 20), /*max_length=*/20, base::ASCIIToUTF16("element"),
+      /*generation_element_id=*/100,
+      /*is_generation_element_password_type=*/true, base::i18n::TextDirection(),
       autofill::PasswordForm());
   auto driver = CreateDriver();
   std::unique_ptr<content::WebContents> web_contents = CreateTestWebContents();

chrome/browser/ui/passwords/password_generation_popup_view_browsertest.cc

@@ -26,11 +26,12 @@ class TestPasswordGenerationPopupController
       : PasswordGenerationPopupControllerImpl(
             gfx::RectF(0, 0, 10, 10),
             autofill::password_generation::PasswordGenerationUIData(
-                gfx::RectF(0, 0, 10, 10),
-                10,
-                base::string16(),
-                100,
-                base::i18n::TextDirection(),
+                /*bounds=*/gfx::RectF(0, 0, 10, 10),
+                /*max_length=*/10,
+                /*generation_element=*/base::string16(),
+                /*generation_element_renderer_id=*/100,
+                /*is_generation_element_password_type=*/true,
+                /*text_direction=*/base::i18n::TextDirection(),
                 PasswordForm()),
             password_manager::ContentPasswordManagerDriverFactory::
                 FromWebContents(web_contents)

components/autofill/content/renderer/password_generation_agent.cc

@@ -307,6 +307,12 @@ void PasswordGenerationAgent::UserTriggeredGeneratePassword(
     UserTriggeredGeneratePasswordCallback callback) {
   if (SetUpUserTriggeredGeneration()) {
     LogMessage(Logger::STRING_GENERATION_RENDERER_SHOW_MANUAL_GENERATION_POPUP);
+    // If the field is not |type=password|, the list of suggestions
+    // should not be populated with passwords to avoid filling them in a
+    // clear-text field.
+    // |IsPasswordFieldForAutofill()| is deliberately not used.
+    bool is_generation_element_password_type =
+        current_generation_item_->generation_element_.IsPasswordField();
     autofill::password_generation::PasswordGenerationUIData
         password_generation_ui_data(
             render_frame()->ElementBoundsInWindow(
@@ -316,6 +322,7 @@ void PasswordGenerationAgent::UserTriggeredGeneratePassword(
                 .Utf16(),
             current_generation_item_->generation_element_
                 .UniqueRendererFormControlId(),
+            is_generation_element_password_type,
             GetTextDirectionForElement(
                 current_generation_item_->generation_element_),
             current_generation_item_->form_);
@@ -511,6 +518,12 @@ void PasswordGenerationAgent::AutomaticGenerationAvailable() {
   DCHECK(current_generation_item_);
   DCHECK(!current_generation_item_->generation_element_.IsNull());
   LogMessage(Logger::STRING_GENERATION_RENDERER_AUTOMATIC_GENERATION_AVAILABLE);
+  // If the field is not |type=password|, the list of suggestions
+  // should not be populated with passwordS to avoid filling them in a
+  // clear-text field.
+  // |IsPasswordFieldForAutofill()| is deliberately not used.
+  bool is_generation_element_password_type =
+      current_generation_item_->generation_element_.IsPasswordField();
   autofill::password_generation::PasswordGenerationUIData
       password_generation_ui_data(
           render_frame()->ElementBoundsInWindow(
@@ -520,6 +533,7 @@ void PasswordGenerationAgent::AutomaticGenerationAvailable() {
               .Utf16(),
           current_generation_item_->generation_element_
               .UniqueRendererFormControlId(),
+          is_generation_element_password_type,
           GetTextDirectionForElement(
               current_generation_item_->generation_element_),
           current_generation_item_->form_);

components/autofill/core/common/mojom/autofill_types.mojom

@@ -214,6 +214,7 @@ struct PasswordGenerationUIData {
   int32 max_length;
   mojo_base.mojom.String16 generation_element;
   uint32 generation_element_id;
+  bool is_generation_element_password_type;
   mojo_base.mojom.TextDirection text_direction;
   PasswordForm password_form;
 };

components/autofill/core/common/mojom/autofill_types_mojom_traits.cc

@@ -223,6 +223,8 @@ bool StructTraits<autofill::mojom::PasswordGenerationUIDataDataView,
 
   out->max_length = data.max_length();
   out->generation_element_id = data.generation_element_id();
+  out->is_generation_element_password_type =
+      data.is_generation_element_password_type();
 
   if (!data.ReadGenerationElement(&out->generation_element) ||
       !data.ReadTextDirection(&out->text_direction) ||

components/autofill/core/common/mojom/autofill_types_mojom_traits.h

@@ -408,6 +408,11 @@ struct StructTraits<autofill::mojom::PasswordGenerationUIDataDataView,
     return r.generation_element_id;
   }
 
+  static bool is_generation_element_password_type(
+      const autofill::password_generation::PasswordGenerationUIData& r) {
+    return r.is_generation_element_password_type;
+  }
+
   static base::i18n::TextDirection text_direction(
       const autofill::password_generation::PasswordGenerationUIData& r) {
     return r.text_direction;

components/autofill/core/common/mojom/autofill_types_mojom_traits_unittest.cc

@@ -117,6 +117,7 @@ void CreatePasswordGenerationUIData(
   data->max_length = 20;
   data->generation_element = base::ASCIIToUTF16("generation_element");
   data->text_direction = base::i18n::RIGHT_TO_LEFT;
+  data->is_generation_element_password_type = false;
   CreateTestPasswordForm(&data->password_form);
 }
 
@@ -154,6 +155,8 @@ void CheckEqualPassPasswordGenerationUIData(
   EXPECT_EQ(expected.bounds, actual.bounds);
   EXPECT_EQ(expected.max_length, actual.max_length);
   EXPECT_EQ(expected.generation_element, actual.generation_element);
+  EXPECT_EQ(expected.is_generation_element_password_type,
+            actual.is_generation_element_password_type);
   EXPECT_EQ(expected.text_direction, actual.text_direction);
   EXPECT_EQ(expected.password_form, actual.password_form);
 }

components/autofill/core/common/password_generation_util.cc

@@ -17,12 +17,14 @@ PasswordGenerationUIData::PasswordGenerationUIData(
     int max_length,
     const base::string16& generation_element,
     uint32_t generation_element_id,
+    bool is_generation_element_password_type,
     base::i18n::TextDirection text_direction,
     const autofill::PasswordForm& password_form)
     : bounds(bounds),
       max_length(max_length),
       generation_element(generation_element),
       generation_element_id(generation_element_id),
+      is_generation_element_password_type(is_generation_element_password_type),
       text_direction(text_direction),
       password_form(password_form) {}
 

components/autofill/core/common/password_generation_util.h

@@ -109,6 +109,7 @@ struct PasswordGenerationUIData {
                            int max_length,
                            const base::string16& generation_element,
                            uint32_t generation_element_id,
+                           bool is_generation_element_password_type,
                            base::i18n::TextDirection text_direction,
                            const autofill::PasswordForm& password_form);
   PasswordGenerationUIData();
@@ -132,6 +133,9 @@ struct PasswordGenerationUIData {
   // Renderer ID of the generation element.
   uint32_t generation_element_id;
 
+  // Is the generation element |type=password|.
+  bool is_generation_element_password_type;
+
   // Direction of the text for |generation_element|.
   base::i18n::TextDirection text_direction;
 

components/password_manager/core/browser/password_autofill_manager.cc

@@ -7,6 +7,7 @@
 #include <stddef.h>
 
 #include <algorithm>
+#include <string>
 #include <utility>
 #include <vector>
 
@@ -338,14 +339,16 @@ bool PasswordAutofillManager::MaybeShowPasswordSuggestions(
 
 bool PasswordAutofillManager::MaybeShowPasswordSuggestionsWithGeneration(
     const gfx::RectF& bounds,
-    base::i18n::TextDirection text_direction) {
+    base::i18n::TextDirection text_direction,
+    bool show_password_suggestions) {
   if (!fill_data_)
     return false;
   std::vector<autofill::Suggestion> suggestions;
-  GetSuggestions(*fill_data_, base::string16(), page_favicon_,
-                 true /* show_all */, true /* is_password_field */,
-                 &suggestions);
-
+  if (show_password_suggestions) {
+    GetSuggestions(*fill_data_, base::string16(), page_favicon_,
+                   true /* show_all */, true /* is_password_field */,
+                   &suggestions);
+  }
   // Add 'Generation' option.
   // The UI code will pick up an icon from the resources based on the string.
   autofill::Suggestion suggestion(

components/password_manager/core/browser/password_autofill_manager.h

@@ -6,6 +6,7 @@
 #define COMPONENTS_PASSWORD_MANAGER_CORE_BROWSER_PASSWORD_AUTOFILL_MANAGER_H_
 
 #include <map>
+#include <memory>
 
 #include "base/callback.h"
 #include "base/i18n/rtl.h"
@@ -85,7 +86,8 @@ class PasswordAutofillManager : public autofill::AutofillPopupDelegate {
   // and returns false.
   bool MaybeShowPasswordSuggestionsWithGeneration(
       const gfx::RectF& bounds,
-      base::i18n::TextDirection text_direction);
+      base::i18n::TextDirection text_direction,
+      bool show_password_suggestions);
 
   // Called when main frame navigates. Not called for in-page navigations.
   void DidNavigateMainFrame();

components/password_manager/core/browser/password_autofill_manager_unittest.cc

@@ -6,6 +6,8 @@
 
 #include <memory>
 #include <string>
+#include <utility>
+#include <vector>
 
 #include "base/command_line.h"
 #include "base/compiler_specific.h"
@@ -299,9 +301,10 @@ TEST_F(PasswordAutofillManagerTest, ExternalDelegatePasswordSuggestions) {
     EXPECT_CALL(*autofill_client, HideAutofillPopup());
     base::HistogramTester histograms;
     password_autofill_manager_->DidAcceptSuggestion(
-        test_username_, is_suggestion_on_password_field
-                            ? autofill::POPUP_ITEM_ID_PASSWORD_ENTRY
-                            : autofill::POPUP_ITEM_ID_USERNAME_ENTRY,
+        test_username_,
+        is_suggestion_on_password_field
+            ? autofill::POPUP_ITEM_ID_PASSWORD_ENTRY
+            : autofill::POPUP_ITEM_ID_USERNAME_ENTRY,
         1);
     histograms.ExpectUniqueSample(
         kDropdownSelectedHistogram,
@@ -735,7 +738,8 @@ TEST_F(PasswordAutofillManagerTest,
   gfx::RectF element_bounds;
   EXPECT_FALSE(
       password_autofill_manager_->MaybeShowPasswordSuggestionsWithGeneration(
-          element_bounds, base::i18n::RIGHT_TO_LEFT));
+          element_bounds, base::i18n::RIGHT_TO_LEFT,
+          /*show_password_suggestions=*/true));
 }
 
 TEST_F(PasswordAutofillManagerTest,
@@ -770,7 +774,8 @@ TEST_F(PasswordAutofillManagerTest,
           false, PopupType::kPasswords, _));
   EXPECT_TRUE(
       password_autofill_manager_->MaybeShowPasswordSuggestionsWithGeneration(
-          element_bounds, base::i18n::RIGHT_TO_LEFT));
+          element_bounds, base::i18n::RIGHT_TO_LEFT,
+          /*show_password_suggestions=*/true));
   histograms.ExpectUniqueSample(
       kDropdownShownHistogram,
       metrics_util::PasswordDropdownState::kStandardGenerate, 1);
@@ -785,4 +790,39 @@ TEST_F(PasswordAutofillManagerTest,
       metrics_util::PasswordDropdownSelectedOption::kGenerate, 1);
 }
 
+TEST_F(PasswordAutofillManagerTest,
+       MaybeShowPasswordSuggestionsWithOmittedCredentials) {
+  auto client = std::make_unique<TestPasswordManagerClient>();
+  auto autofill_client = std::make_unique<MockAutofillClient>();
+  InitializePasswordAutofillManager(client.get(), autofill_client.get());
+
+  gfx::RectF element_bounds;
+  autofill::PasswordFormFillData data;
+  data.username_field.value = test_username_;
+  data.password_field.value = test_password_;
+  data.origin = GURL("https://foo.test");
+
+  favicon::MockFaviconService favicon_service;
+  EXPECT_CALL(*client, GetFaviconService()).WillOnce(Return(&favicon_service));
+  EXPECT_CALL(favicon_service, GetFaviconImageForPageURL(data.origin, _, _));
+  password_autofill_manager_->OnAddPasswordFillData(data);
+
+  base::string16 generation_string =
+      l10n_util::GetStringUTF16(IDS_PASSWORD_MANAGER_GENERATE_PASSWORD);
+
+  EXPECT_CALL(
+      *autofill_client,
+      ShowAutofillPopup(
+          element_bounds, base::i18n::RIGHT_TO_LEFT,
+          AllOf(SuggestionVectorValuesAre(
+                    ElementsAreArray(GetSuggestionList({generation_string}))),
+                SuggestionVectorIconsAre(
+                    ElementsAreArray(GetIconsList({"keyIcon"})))),
+          false, PopupType::kPasswords, _));
+
+  EXPECT_TRUE(
+      password_autofill_manager_->MaybeShowPasswordSuggestionsWithGeneration(
+          element_bounds, base::i18n::RIGHT_TO_LEFT,
+          /*show_password_suggestions=*/false));
+}
 }  // namespace password_manager