Prevent autofilling credit card security number fields with passwords.

components/autofill/content/renderer/password_form_conversion_utils.cc

Index: components/autofill/content/renderer/password_form_conversion_utils.cc
diff --git a/components/autofill/content/renderer/password_form_conversion_utils.cc b/components/autofill/content/renderer/password_form_conversion_utils.cc
index 1a0f3d7ed8f9ecb88bbbe0dff1ecc03cfe364dc1..515f73bb19f884efa2c12ca39ae9f96c3dd1bb65 100644
--- a/components/autofill/content/renderer/password_form_conversion_utils.cc
+++ b/components/autofill/content/renderer/password_form_conversion_utils.cc
@@ -19,6 +19,8 @@
 #include "base/strings/string_util.h"
 #include "base/strings/utf_string_conversions.h"
 #include "components/autofill/content/renderer/form_autofill_util.h"
+#include "components/autofill/core/common/autofill_regex_constants.h"
+#include "components/autofill/core/common/autofill_regexes.h"
 #include "components/autofill/core/common/autofill_util.h"
 #include "components/autofill/core/common/password_form.h"
 #include "components/autofill/core/common/password_form_field_prediction_map.h"
@@ -429,6 +431,8 @@ bool GetPasswordForm(
 
     if (HasCreditCardAutocompleteAttributes(*input_element))
       continue;
+    if (IsCreditCardVerificationField(*input_element))
+      continue;
 
     bool element_is_invisible = !form_util::IsWebElementVisible(*input_element);
     if (input_element->IsTextField()) {
@@ -766,4 +770,14 @@ bool HasCreditCardAutocompleteAttributes(
   return false;
 }
 
+bool IsCreditCardVerificationField(const blink::WebInputElement& field) {

[Mathieu, 2017/05/11 12:54:03]

IsCreditCardVerificationAndPasswordField?

[pkalinnikov, 2017/05/11 14:03:07]

I think it looks better without "And". WDYT?

[Mathieu, 2017/05/11 15:40:08]

The logic of this function is "returns true if the field is a password field AND
a credit card verification field". Right now the name only reflects the latter,
so that's why I was suggesting a better name. Removing And is fine by me, it's a
matter of taste :)

[pkalinnikov, 2017/05/12 08:42:05]

Done in patch#2.

+  if (!field.IsPasswordField())
+    return false;
+
+  static const base::string16 kCardCvcReCached = base::UTF8ToUTF16(kCardCvcRe);

[dvadym, 2017/05/11 11:46:53]

Probably it makes sense to use another regexps for finding other credit card
fields, for example kCardNumberRe etc. 
Mathieu, WDYT filtering out of which autofill types by Password Manager would be
helpful?

[Mathieu, 2017/05/11 12:54:03]

We've really only seen this problem with CVC fields that are also password
fields. Have you seen it with others?

[dvadym, 2017/05/11 14:27:51]

I didn't see any other type for password fields. But I meant that we can filter
out for example text fields for credit card numbers.

[Mathieu, 2017/05/11 15:40:08]

Text fields that would be type=password and contain credit card numbers? Sure,
we can also do that. It's defensive but better than nothing.

[dvadym, 2017/05/11 15:50:18]

I mean type=text, i.e. to expand this function to checking not only password
fields or introduce another function for checking fields with type=text.

+
+  return MatchesPattern(field.GetAttribute("id").Utf16(), kCardCvcReCached) ||

[Mathieu, 2017/05/11 12:54:04]

I'm pretty sure this will cache the patterns, so it's fine to use
MatchesPattern(..., kCardCvcRe)

[pkalinnikov, 2017/05/11 14:03:07]

Can you elaborate on what you mean by "this will cache the patterns"? I think
the AutofillRegexes class under the hood caches them in a different sense.

MatchesPattern expects the second parameter to be base::string16, and direct
passing of kCardCvcRe doesn't compile. Converting to base::string16 is a
necessary step.

I could make it non-static, but this will result in a memory allocation every
time the function is called.

[Mathieu, 2017/05/11 15:40:08]

you can do UTF8ToUTF16(kCardCvcRe), no? That's what other code seems to be
doing:
https://cs.chromium.org/chromium/src/components/autofill/core/browser/address...

[pkalinnikov, 2017/05/12 08:42:05]

Well, I do the same. But instead of storing the value on stack, I put it to
static, and thus only one allocation is needed instead of tons of allocations,
one per IsCreditCardVerificationPasswordField invocation (that is, for each
field in each form).

Probably the other code could also consider putting these constants to statics
for better performance. The ideal solution would be after all to make the
constants in autofill_regex_constants.h base::string16 right away, since no code
seems to be using them as const char* in the first place. Check this out:
https://goo.gl/DBZvFx. Only autofill/core/browser/phone_field.cc manipulates
them as std::strings, but in the end it produces a string16 constant for
matching anyway, so this could also be done with string16 operators.

[Mathieu, 2017/05/12 12:24:44]

Can you file a bug for the general behavior you describe and include rogerm@ in
the discussion?

[pkalinnikov, 2017/05/12 14:48:21]

Will do.

+         MatchesPattern(field.GetAttribute("name").Utf16(), kCardCvcReCached);
+}
+
 }  // namespace autofill