Failure to show save prompt on www.shoedazzle.com due to misclassification of password field as CVC |
|||||
Issue descriptionWe fail to show the password save prompt on www.shoedazzle.com. The reason for this is the following: In password_form_conversion_utils.cc we check whether the input name matches a CVC regex: https://cs.chromium.org/chromium/src/components/autofill/content/renderer/password_form_conversion_utils.cc?l=518&rcl=bf0112123d055721209dfe0c0f24b5774918ea9e The field's name ("verification_type") matches the regex, therefore, the password manager sets password_form->only_for_fallback_saving https://cs.chromium.org/chromium/src/components/autofill/content/renderer/password_form_conversion_utils.cc?l=698&rcl=bf0112123d055721209dfe0c0f24b5774918ea9e and does not save. When investigating, we noticed that the new form parser does not contain this logic of checking the CVC regex against input field names. We have the following choices: 1) Do not implement this feature for the new parser. shoedazlle.com would be fixed, but we cannot estimate the negative effort (which would mean that we ask the user to save a password even though they have just entered their credit card). 2) Implement this feature for the new parser and rely on crowdsourcing to override it. This is more conservative in the sense that we don't change behavior that we cannot measure. However, the new parser can still perform better than the current one because we allow server-side overrides. The signal we would use for that is whether people use the manual fallback. In this particular case we have seen several users using the manual fallback, so there should be some signal - at least for the short tail. ==> Conclusion: Go with option 2 as we cannot estimate the impact of the regression from 1.
,
Jul 18
The most recent update is that * We will try to start sending signals identifying CVC fields so that autofill server can actually classify them (it does not do so right now). * We will implement solution 2) from the bug description as a short-time measure until the server-side solution works.
,
Jul 18
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/fe5e8f32dde36fcb094ba07bf4f27076108eca2e commit fe5e8f32dde36fcb094ba07bf4f27076108eca2e Author: Vaclav Brozek <vabr@chromium.org> Date: Wed Jul 18 16:09:13 2018 Teach FormData->PasswordForm parser CVC The new FormData->PasswordForm parser did not check fields for being CVC fields (except for autocomplete attributes). Ideally, it would not need to, because it could rely on server hints, but the hints are not ready yet, so this CL introduces a short-term ability for the new parser to understand when a field name sounds like that of a CVC field. While in the particular case of the linked bug (on http://www.shoedazzle.com/), this will break the previously correct behaviour of the parser, the goal is to minimize the difference in behaviour against the old parser, so as not to introduce an unknown amount of new breakages. Bug: 860700 Change-Id: I4595dbf01142e9f26f7264a1d62cde4cf5e4c0c0 Reviewed-on: https://chromium-review.googlesource.com/1141958 Reviewed-by: Vadym Doroshenko <dvadym@chromium.org> Commit-Queue: Vaclav Brozek <vabr@chromium.org> Cr-Commit-Position: refs/heads/master@{#576082} [modify] https://crrev.com/fe5e8f32dde36fcb094ba07bf4f27076108eca2e/components/password_manager/core/browser/form_parsing/form_parser.cc [modify] https://crrev.com/fe5e8f32dde36fcb094ba07bf4f27076108eca2e/components/password_manager/core/browser/form_parsing/form_parser_unittest.cc
,
Sep 14
The design for the server-side solution is being written at https://docs.google.com/document/d/15VCazN-8FjeApxK66zoidLEgbJMGNAWekR22vfVxeDA/edit?usp=sharing (sorry, Google-internal).
,
Oct 22
,
Oct 26
,
Oct 26
The general work here was transferred to issue 899331. That bug has a TODO to check if those works result in fixing this bug. There is no other action to take here, so I'll disown this, but keep it open until we can confirm that bug 899331 fixed this. |
|||||
►
Sign in to add a comment |
|||||
Comment 1 by battre@chromium.org
, Jul 6