Prevent autofilling credit card security number fields with passwords.

components/autofill/content/renderer/password_form_conversion_utils.cc

 // Copyright 2013 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/autofill/content/renderer/password_form_conversion_utils.h"
 
 #include <stddef.h>
 
 #include <algorithm>
 #include <string>
 
 #include "base/i18n/case_conversion.h"
 #include "base/lazy_instance.h"
 #include "base/macros.h"
 #include "base/metrics/histogram_macros.h"
 #include "base/stl_util.h"
 #include "base/strings/string16.h"
 #include "base/strings/string_split.h"
 #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"
 #include "google_apis/gaia/gaia_urls.h"
 #include "third_party/WebKit/public/platform/WebString.h"
 #include "third_party/WebKit/public/platform/WebVector.h"
 #include "third_party/WebKit/public/web/WebDocument.h"
 #include "third_party/WebKit/public/web/WebFormControlElement.h"
 #include "third_party/WebKit/public/web/WebFrame.h"
 #include "third_party/WebKit/public/web/WebInputElement.h"
@@ skipping 390 matching lines @@
   size_t number_of_non_empty_text_non_password_fields = 0;
   for (size_t i = 0; i < form.control_elements.size(); ++i) {
     WebFormControlElement control_element = form.control_elements[i];
 
     WebInputElement* input_element = ToWebInputElement(&control_element);
     if (!input_element || !input_element->IsEnabled())
       continue;
 
     if (HasCreditCardAutocompleteAttributes(*input_element))
       continue;
+    if (IsCreditCardVerificationField(*input_element))
+      continue;
 
     bool element_is_invisible = !form_util::IsWebElementVisible(*input_element);
     if (input_element->IsTextField()) {
       if (input_element->IsPasswordField()) {
         if (element_is_invisible && ignore_invisible_passwords)
           continue;
         layout_sequence.push_back('P');
       } else {
         if (FieldHasNonscriptModifiedValue(field_value_and_properties_map,
                                            *input_element))
@@ skipping 317 matching lines @@
            autocomplete_value_lowercase, base::kWhitespaceASCII,
            base::TRIM_WHITESPACE, base::SPLIT_WANT_NONEMPTY)) {
     if (base::StartsWith(token, kAutocompleteCreditCardPrefix,
                          base::CompareCase::SENSITIVE)) {
       return true;
     }
   }
   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