Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2874803002:
Prevent autofilling credit card security number fields with passwords. (Closed)
|
Created:
3 years, 7 months ago by pkalinnikov Modified:
3 years, 7 months ago CC:
chromium-reviews, mlamouri+watch-content_chromium.org, jam, vabr+watchlistpasswordmanager_chromium.org, rouslan+autofill_chromium.org, rogerm+autofillwatch_chromium.org, sebsg+autofillwatch_chromium.org, browser-components-watch_chromium.org, mathp+autofillwatch_chromium.org, darin-cc_chromium.org, vabr+watchlistautofill_chromium.org, estade+watch_chromium.org, gcasto+watchlist_chromium.org Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionPrevent autofilling credit card security number fields with passwords.
Currently, if a security code input field (with type="password") in a credit
card form is encountered, and there is a password stored for the site, then
autofill completes it with the stored password.
This CL adds a client-side heuristic (i.e., name/id of the field matches a
certain regexp) to filter out such fields from a password form.
BUG=674151
Review-Url: https://codereview.chromium.org/2874803002
Cr-Commit-Position: refs/heads/master@{#471394}
Committed: https://chromium.googlesource.com/chromium/src/+/e2adcd849d16ed6935ff8734f075bfdf729297e4
Patch Set 1 #
Total comments: 15
Patch Set 2 : Address comments from mathp@. #Patch Set 3 : Fix build. #
Messages
Total messages: 44 (28 generated)
The CQ bit was checked by pkalinnikov@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...) win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
The CQ bit was checked by pkalinnikov@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Patchset #1 (id:1) has been deleted
The CQ bit was checked by pkalinnikov@chromium.org to run a CQ dry run
Patchset #1 (id:20001) has been deleted
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== Prevent autofilling for credit card security number. BUG=674151 ========== to ========== Prevent autofilling credit card security number fields with passwords. Currently, if a security code input field (with type="password") in a credit card form is encountered, and there is a password stored for the site, then autofill completes it with the stored password. This CL adds a client-side heuristic (i.e., name/id of the field matches a certain regexp) to filter out such fields from a password form.r. BUG=674151 ==========
Description was changed from ========== Prevent autofilling credit card security number fields with passwords. Currently, if a security code input field (with type="password") in a credit card form is encountered, and there is a password stored for the site, then autofill completes it with the stored password. This CL adds a client-side heuristic (i.e., name/id of the field matches a certain regexp) to filter out such fields from a password form.r. BUG=674151 ========== to ========== Prevent autofilling credit card security number fields with passwords. Currently, if a security code input field (with type="password") in a credit card form is encountered, and there is a password stored for the site, then autofill completes it with the stored password. This CL adds a client-side heuristic (i.e., name/id of the field matches a certain regexp) to filter out such fields from a password form. BUG=674151 ==========
pkalinnikov@chromium.org changed reviewers: + dvadym@chromium.org, kolos@chromium.org, mathp@chromium.org
PTAL.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Thanks Pavel for implementing this! Implementation from Password Manager point of view looks good. Mathieu, does the current implementation make sense from Autofill point of view? Should we implement checking of more Autofill types? https://codereview.chromium.org/2874803002/diff/40001/components/autofill/con... File components/autofill/content/renderer/password_form_conversion_utils.cc (right): https://codereview.chromium.org/2874803002/diff/40001/components/autofill/con... components/autofill/content/renderer/password_form_conversion_utils.cc:777: static const base::string16 kCardCvcReCached = base::UTF8ToUTF16(kCardCvcRe); 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?
https://codereview.chromium.org/2874803002/diff/40001/components/autofill/con... File components/autofill/content/renderer/password_form_conversion_utils.cc (right): https://codereview.chromium.org/2874803002/diff/40001/components/autofill/con... components/autofill/content/renderer/password_form_conversion_utils.cc:773: bool IsCreditCardVerificationField(const blink::WebInputElement& field) { IsCreditCardVerificationAndPasswordField? https://codereview.chromium.org/2874803002/diff/40001/components/autofill/con... components/autofill/content/renderer/password_form_conversion_utils.cc:777: static const base::string16 kCardCvcReCached = base::UTF8ToUTF16(kCardCvcRe); On 2017/05/11 11:46:53, dvadym wrote: > 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? We've really only seen this problem with CVC fields that are also password fields. Have you seen it with others? https://codereview.chromium.org/2874803002/diff/40001/components/autofill/con... components/autofill/content/renderer/password_form_conversion_utils.cc:779: return MatchesPattern(field.GetAttribute("id").Utf16(), kCardCvcReCached) || I'm pretty sure this will cache the patterns, so it's fine to use MatchesPattern(..., kCardCvcRe)
The CQ bit was checked by pkalinnikov@chromium.org to run a CQ dry run
https://codereview.chromium.org/2874803002/diff/40001/components/autofill/con... File components/autofill/content/renderer/password_form_conversion_utils.cc (right): https://codereview.chromium.org/2874803002/diff/40001/components/autofill/con... components/autofill/content/renderer/password_form_conversion_utils.cc:773: bool IsCreditCardVerificationField(const blink::WebInputElement& field) { On 2017/05/11 12:54:03, Mathieu wrote: > IsCreditCardVerificationAndPasswordField? I think it looks better without "And". WDYT? https://codereview.chromium.org/2874803002/diff/40001/components/autofill/con... components/autofill/content/renderer/password_form_conversion_utils.cc:779: return MatchesPattern(field.GetAttribute("id").Utf16(), kCardCvcReCached) || On 2017/05/11 12:54:04, Mathieu wrote: > I'm pretty sure this will cache the patterns, so it's fine to use > MatchesPattern(..., kCardCvcRe) 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.
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by pkalinnikov@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2874803002/diff/40001/components/autofill/con... File components/autofill/content/renderer/password_form_conversion_utils.cc (right): https://codereview.chromium.org/2874803002/diff/40001/components/autofill/con... components/autofill/content/renderer/password_form_conversion_utils.cc:777: static const base::string16 kCardCvcReCached = base::UTF8ToUTF16(kCardCvcRe); On 2017/05/11 12:54:03, Mathieu wrote: > On 2017/05/11 11:46:53, dvadym wrote: > > 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? > > We've really only seen this problem with CVC fields that are also password > fields. Have you seen it with others? 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.
https://codereview.chromium.org/2874803002/diff/40001/components/autofill/con... File components/autofill/content/renderer/password_form_conversion_utils.cc (right): https://codereview.chromium.org/2874803002/diff/40001/components/autofill/con... components/autofill/content/renderer/password_form_conversion_utils.cc:773: bool IsCreditCardVerificationField(const blink::WebInputElement& field) { On 2017/05/11 14:03:07, pkalinnikov wrote: > On 2017/05/11 12:54:03, Mathieu wrote: > > IsCreditCardVerificationAndPasswordField? > > I think it looks better without "And". WDYT? 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 :) https://codereview.chromium.org/2874803002/diff/40001/components/autofill/con... components/autofill/content/renderer/password_form_conversion_utils.cc:777: static const base::string16 kCardCvcReCached = base::UTF8ToUTF16(kCardCvcRe); On 2017/05/11 14:27:51, dvadym wrote: > On 2017/05/11 12:54:03, Mathieu wrote: > > On 2017/05/11 11:46:53, dvadym wrote: > > > 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? > > > > We've really only seen this problem with CVC fields that are also password > > fields. Have you seen it with others? > > 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. 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. https://codereview.chromium.org/2874803002/diff/40001/components/autofill/con... components/autofill/content/renderer/password_form_conversion_utils.cc:779: return MatchesPattern(field.GetAttribute("id").Utf16(), kCardCvcReCached) || On 2017/05/11 14:03:07, pkalinnikov wrote: > On 2017/05/11 12:54:04, Mathieu wrote: > > I'm pretty sure this will cache the patterns, so it's fine to use > > MatchesPattern(..., kCardCvcRe) > > 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. 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...
https://codereview.chromium.org/2874803002/diff/40001/components/autofill/con... File components/autofill/content/renderer/password_form_conversion_utils.cc (right): https://codereview.chromium.org/2874803002/diff/40001/components/autofill/con... components/autofill/content/renderer/password_form_conversion_utils.cc:777: static const base::string16 kCardCvcReCached = base::UTF8ToUTF16(kCardCvcRe); On 2017/05/11 15:40:08, Mathieu wrote: > On 2017/05/11 14:27:51, dvadym wrote: > > On 2017/05/11 12:54:03, Mathieu wrote: > > > On 2017/05/11 11:46:53, dvadym wrote: > > > > 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? > > > > > > We've really only seen this problem with CVC fields that are also password > > > fields. Have you seen it with others? > > > > 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. > > 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. 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.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
I thought once more about non-password fields for username detection, probably it's enough to process only password fields. LGTM from Password Manager point of view.
LGTM Thanks Pavel for fixing that! Checking text fields: That's a good point Vadym. I added it to the list of possible improvements, but let's do it in a separate CL.
https://codereview.chromium.org/2874803002/diff/40001/components/autofill/con... File components/autofill/content/renderer/password_form_conversion_utils.cc (right): https://codereview.chromium.org/2874803002/diff/40001/components/autofill/con... components/autofill/content/renderer/password_form_conversion_utils.cc:773: bool IsCreditCardVerificationField(const blink::WebInputElement& field) { On 2017/05/11 15:40:08, Mathieu wrote: > On 2017/05/11 14:03:07, pkalinnikov wrote: > > On 2017/05/11 12:54:03, Mathieu wrote: > > > IsCreditCardVerificationAndPasswordField? > > > > I think it looks better without "And". WDYT? > > 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 :) Done in patch#2. https://codereview.chromium.org/2874803002/diff/40001/components/autofill/con... components/autofill/content/renderer/password_form_conversion_utils.cc:779: return MatchesPattern(field.GetAttribute("id").Utf16(), kCardCvcReCached) || On 2017/05/11 15:40:08, Mathieu wrote: > On 2017/05/11 14:03:07, pkalinnikov wrote: > > On 2017/05/11 12:54:04, Mathieu wrote: > > > I'm pretty sure this will cache the patterns, so it's fine to use > > > MatchesPattern(..., kCardCvcRe) > > > > 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. > > 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... 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.
lgtm with comment https://codereview.chromium.org/2874803002/diff/40001/components/autofill/con... File components/autofill/content/renderer/password_form_conversion_utils.cc (right): https://codereview.chromium.org/2874803002/diff/40001/components/autofill/con... components/autofill/content/renderer/password_form_conversion_utils.cc:779: return MatchesPattern(field.GetAttribute("id").Utf16(), kCardCvcReCached) || On 2017/05/12 08:42:05, pkalinnikov wrote: > On 2017/05/11 15:40:08, Mathieu wrote: > > On 2017/05/11 14:03:07, pkalinnikov wrote: > > > On 2017/05/11 12:54:04, Mathieu wrote: > > > > I'm pretty sure this will cache the patterns, so it's fine to use > > > > MatchesPattern(..., kCardCvcRe) > > > > > > 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. > > > > 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... > > 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. Can you file a bug for the general behavior you describe and include rogerm@ in the discussion?
Submitting. https://codereview.chromium.org/2874803002/diff/40001/components/autofill/con... File components/autofill/content/renderer/password_form_conversion_utils.cc (right): https://codereview.chromium.org/2874803002/diff/40001/components/autofill/con... components/autofill/content/renderer/password_form_conversion_utils.cc:779: return MatchesPattern(field.GetAttribute("id").Utf16(), kCardCvcReCached) || On 2017/05/12 12:24:44, Mathieu wrote: > On 2017/05/12 08:42:05, pkalinnikov wrote: > > On 2017/05/11 15:40:08, Mathieu wrote: > > > On 2017/05/11 14:03:07, pkalinnikov wrote: > > > > On 2017/05/11 12:54:04, Mathieu wrote: > > > > > I'm pretty sure this will cache the patterns, so it's fine to use > > > > > MatchesPattern(..., kCardCvcRe) > > > > > > > > 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. > > > > > > 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... > > > > 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. > > Can you file a bug for the general behavior you describe and include rogerm@ in > the discussion? Will do.
The CQ bit was checked by pkalinnikov@chromium.org
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
The CQ bit was checked by pkalinnikov@chromium.org
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 80001, "attempt_start_ts": 1494610905023250,
"parent_rev": "f6c17fed3694e3d76683cdb3ac652803f5aa3c47", "commit_rev":
"e2adcd849d16ed6935ff8734f075bfdf729297e4"}
Message was sent while issue was closed.
Description was changed from ========== Prevent autofilling credit card security number fields with passwords. Currently, if a security code input field (with type="password") in a credit card form is encountered, and there is a password stored for the site, then autofill completes it with the stored password. This CL adds a client-side heuristic (i.e., name/id of the field matches a certain regexp) to filter out such fields from a password form. BUG=674151 ========== to ========== Prevent autofilling credit card security number fields with passwords. Currently, if a security code input field (with type="password") in a credit card form is encountered, and there is a password stored for the site, then autofill completes it with the stored password. This CL adds a client-side heuristic (i.e., name/id of the field matches a certain regexp) to filter out such fields from a password form. BUG=674151 Review-Url: https://codereview.chromium.org/2874803002 Cr-Commit-Position: refs/heads/master@{#471394} Committed: https://chromium.googlesource.com/chromium/src/+/e2adcd849d16ed6935ff8734f075... ==========
Message was sent while issue was closed.
Committed patchset #3 (id:80001) as https://chromium.googlesource.com/chromium/src/+/e2adcd849d16ed6935ff8734f075... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
