Commit 1c293967 authored by Jinho Bang's avatar Jinho Bang Committed by Commit Bot

PasswordManager: Use |typed_value| as a fallback if |value| is empty

There is a bug that the save prompt is not shown if the password value
is set as an empty value by JavaScript before form submitted. The bug is
a regression since crrev.com/c/2029514 (M81 stable), but it is enough
reasonable. So, this patch fixes FormParser instead of reverting the CL.

In the current implementation, FormParser is treated as parsing error if
|value| is empty. On the other hand, according to crrev.com/c/1404093,
if |typed_value| is present, we preferentially use it rather than
|value|. Similarly, this patch makes FormParser use |typed value|
preferentially even if |value| is empty.

Reproducible sites:
  - https://m.naver.com
  - https://zino.dev (This is a minimal test page I wrote)

Bug: 1042196
Change-Id: I6652822fd7fe268b54647434c23c68d41abf5010
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2154644Reviewed-by: default avatarVadym Doroshenko  <dvadym@chromium.org>
Commit-Queue: Vadym Doroshenko  <dvadym@chromium.org>
Cr-Commit-Position: refs/heads/master@{#760543}
parent d953e0f3
...@@ -229,7 +229,7 @@ bool IsProbablyNotUsername(const base::string16& s) { ...@@ -229,7 +229,7 @@ bool IsProbablyNotUsername(const base::string16& s) {
} }
// Returns |typed_value| if it is not empty, |value| otherwise. // Returns |typed_value| if it is not empty, |value| otherwise.
base::string16 GetFieldValue(const FormFieldData& field) { const base::string16& GetFieldValue(const FormFieldData& field) {
return field.typed_value.empty() ? field.value : field.typed_value; return field.typed_value.empty() ? field.value : field.typed_value;
} }
...@@ -554,7 +554,7 @@ std::vector<const FormFieldData*> GetRelevantPasswords( ...@@ -554,7 +554,7 @@ std::vector<const FormFieldData*> GetRelevantPasswords(
if (mode == FormDataParser::Mode::kSaving) { if (mode == FormDataParser::Mode::kSaving) {
// Step 2: apply filter criterion (2). // Step 2: apply filter criterion (2).
base::EraseIf(passwords, [](const ProcessedField* processed_field) { base::EraseIf(passwords, [](const ProcessedField* processed_field) {
return processed_field->field->value.empty(); return GetFieldValue(*processed_field->field).empty();
}); });
} }
...@@ -875,21 +875,23 @@ std::vector<ProcessedField> ProcessFields( ...@@ -875,21 +875,23 @@ std::vector<ProcessedField> ProcessFields(
for (const FormFieldData& field : fields) { for (const FormFieldData& field : fields) {
if (!field.IsTextInputElement()) if (!field.IsTextInputElement())
continue; continue;
if (consider_only_non_empty && field.value.empty())
const base::string16& field_value = GetFieldValue(field);
if (consider_only_non_empty && field_value.empty())
continue; continue;
const bool is_password = field.form_control_type == "password"; const bool is_password = field.form_control_type == "password";
if (!field.value.empty()) { if (!field_value.empty()) {
std::set<base::StringPiece16>& seen_values = std::set<base::StringPiece16>& seen_values =
is_password ? seen_password_values : seen_username_values; is_password ? seen_password_values : seen_username_values;
autofill::ValueElementVector* all_possible_fields = autofill::ValueElementVector* all_possible_fields =
is_password ? all_possible_passwords : all_possible_usernames; is_password ? all_possible_passwords : all_possible_usernames;
// Only the field name of the first occurrence is added. // Only the field name of the first occurrence is added.
auto insertion = seen_values.insert(base::StringPiece16(field.value)); auto insertion = seen_values.insert(field_value);
if (insertion.second) { if (insertion.second) {
// There was no such element in |seen_values|. // There was no such element in |seen_values|.
all_possible_fields->push_back({field.value, field.name}); all_possible_fields->push_back({field_value, field.name});
} }
} }
......
...@@ -2412,8 +2412,9 @@ TEST(FormParserTest, GetSignonRealm) { ...@@ -2412,8 +2412,9 @@ TEST(FormParserTest, GetSignonRealm) {
} }
TEST(FormParserTest, TypedValues) { TEST(FormParserTest, TypedValues) {
CheckTestData({{ CheckTestData({
.description_for_logging = "Simple sign-in forms with typed values", {
.description_for_logging = "Form with changed by JavaScript values",
// Tests that typed values are taken as username, password and // Tests that typed values are taken as username, password and
// new password instead of values that are set by JavaScript. // new password instead of values that are set by JavaScript.
.fields = .fields =
...@@ -2434,7 +2435,31 @@ TEST(FormParserTest, TypedValues) { ...@@ -2434,7 +2435,31 @@ TEST(FormParserTest, TypedValues) {
.typed_value = "typed_new_password", .typed_value = "typed_new_password",
.form_control_type = "password"}, .form_control_type = "password"},
}, },
}}); },
{
.description_for_logging = "Form with cleared by JavaScript values",
// Tests that typed values are taken as username, password and
// new password instead of values that are cleared by JavaScript.
.fields =
{
{.role = ElementRole::USERNAME,
.autocomplete_attribute = "username",
.value = "",
.typed_value = "typed_username",
.form_control_type = "text"},
{.role = ElementRole::CURRENT_PASSWORD,
.autocomplete_attribute = "current-password",
.value = "",
.typed_value = "typed_password",
.form_control_type = "password"},
{.role = ElementRole::NEW_PASSWORD,
.autocomplete_attribute = "new-password",
.value = "",
.typed_value = "typed_new_password",
.form_control_type = "password"},
},
},
});
} }
TEST(FormParserTest, ContradictingPasswordPredictionAndAutocomplete) { TEST(FormParserTest, ContradictingPasswordPredictionAndAutocomplete) {
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment