Password generation with identical password fields |
||
Issue descriptionA could of sites have a structure like this in the sign-up form: (example taken from https://cloud.tencent.com/register) <input type="password" data-id="passwordInput"> <input type="password" data-id="confirmPasswordInput"> Because the password fields have neither a name nor an id attribute, their field signatures are identical. Even if the server recognizes them as new-password fields, Chrome offers password generation only on the second field, not on the first. Would any of this make sense? 1) If we have two fields with identical field signature and type 76, offer password generation on the first and consider the second field as a confirmation field. --> Probably a change in the parser. 2) Change the PasswordGenerationAgent to offer generation on the second field instead of the first. 3) Use a different field signature in case of collisions. Sample sites that I know: https://cloud.tencent.com/register https://orionx.com/register https://konto-pocztowe.interia.pl/#/nowe-konto
,
Nov 7
Another example https://login.salliemae.com
,
Nov 8
If it helps, changing the parser to always accept the first "new-password" hint is trivial, I can do it immediately.
,
Nov 8
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/e9e7065b5341222f06b176ca2df5c5756e34d653 commit e9e7065b5341222f06b176ca2df5c5756e34d653 Author: Vaclav Brozek <vabr@chromium.org> Date: Thu Nov 08 11:21:15 2018 Check multiple roles in form_parser_unittest.cc The test expectations for form_parser_unittest.cc contain the expected roles for form fields (username, new-password, etc.). There can be at most one of each, so if there are multiple same roles, all but one are ignored. This CL adds a DCHECK to guard that there are no multiple same roles specified, so that the expecations are not described in a confusing way. It is a DCHECK instead of an ASSERT/EXPECT, because this is something not depending on the tested code, just the way the test is written. Hence if it passes when submitting the CL, it will pass until the next change to the test file. Bug: 902700 Change-Id: I022daf5a1701f13e7d52c881b60acfc412cd2e96 Reviewed-on: https://chromium-review.googlesource.com/c/1325985 Reviewed-by: Vadym Doroshenko <dvadym@chromium.org> Commit-Queue: Vaclav Brozek <vabr@chromium.org> Cr-Commit-Position: refs/heads/master@{#606414} [modify] https://crrev.com/e9e7065b5341222f06b176ca2df5c5756e34d653/components/password_manager/core/browser/form_parsing/form_parser_unittest.cc
,
Nov 8
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/32b1bd2754246bd4e95ea9c5b21e7f54e2dc836c commit 32b1bd2754246bd4e95ea9c5b21e7f54e2dc836c Author: Vaclav Brozek <vabr@chromium.org> Date: Thu Nov 08 12:56:22 2018 Recognise first hinted new-password field The FormData -> PasswordForm parser can see multiple hints from the server that a field is a new-password field. In particular, this happens if those fields have the same signature. The parser only can choose one new-parser field, and on that field, password generation is offered. If this is any other than the first field, the user has likely already thought of and typed their new password elsewhere. Therefore, this CL makes sure that the first hinted new-password field is marked as such by the parser. Bug: 902700 Change-Id: Idb065e7391526036347feb586b2a702d74f431a9 Reviewed-on: https://chromium-review.googlesource.com/c/1325986 Reviewed-by: Dominic Battré <battre@chromium.org> Commit-Queue: Vaclav Brozek <vabr@chromium.org> Cr-Commit-Position: refs/heads/master@{#606430} [modify] https://crrev.com/32b1bd2754246bd4e95ea9c5b21e7f54e2dc836c/components/password_manager/core/browser/form_parsing/form_parser.cc [modify] https://crrev.com/32b1bd2754246bd4e95ea9c5b21e7f54e2dc836c/components/password_manager/core/browser/form_parsing/form_parser_unittest.cc
,
Nov 8
With r606430 (#5 above), first half of point 1) from the description is now done.
,
Nov 8
The missing half of point 1) is: "consider the second field as a confirmation field". It is easy to do, and exactly what we do for two "autocomplete='new-password'" fields, but not sure if it changes anything in the case of server hints. Looking at where PasswordForm::confirmation_password_element is used, I only see votes_uploader.cc. I would expect that confirmation_password_element is useful for generation code to recognise where to copy the generated password value to, but could not find it in the code. Vadym/Maxim, do you know, whether PasswordForm::confirmation_password_element is useful enough to change the parser to populate it with the second "new-password" hint?
,
Nov 8
After talking to Vadym, I can answer my question from #7: confirmation_password_element is not yet used for generation, but is planned to be used really soon. Therefore finishing step 1) makes sense and I'll do it.
,
Nov 9
The task mentioned in #8 is implemented in https://crrev.com/c/1326511. However, there is an alternative implementation ([1]) which I'd like to discuss with Vadym next Thursday. So I'm postponing landing that until end of next week. [1] https://chromium-review.googlesource.com/c/chromium/src/+/1326511#message-b4977e06f7e5ff4b45009144d2f56602bf3e5f53
,
Nov 16
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/eeeaa95d986b05dad65fc40e803fc14ebe040354 commit eeeaa95d986b05dad65fc40e803fc14ebe040354 Author: Vaclav Brozek <vabr@chromium.org> Date: Fri Nov 16 17:15:20 2018 Recognise second hinted new-password field as confirmation Chrome can see multiple hints from the server that a field is a new-password field. This happens, in particular, if two fields have the same name and hence the same signature -- because server hints are keyed by the signature, there is no way to specify two different types for two fields with the same name. If there is no other explicitly hinted confirmation field, then it is likely that in reality one of the new-password fields was supposed to be a confirmation field. Knowing which field is the confirmation field will be useful for Chrome to understand where to copy the generated password to. Therefore, this CL makes sure that the second hinted same-signature new-password field is marked as a confirmation field, unless the server also explicitly hints that some field is a confirmation field. This is analogous to what the parser already does in case of two fields with the autocomplete="new-password" attribute. Bug: 902700 Change-Id: I06e75a6d1059976c68d04903351d9bd207de2ee9 Reviewed-on: https://chromium-review.googlesource.com/c/1326511 Commit-Queue: Vaclav Brozek <vabr@chromium.org> Reviewed-by: Vadym Doroshenko <dvadym@chromium.org> Cr-Commit-Position: refs/heads/master@{#608818} [modify] https://crrev.com/eeeaa95d986b05dad65fc40e803fc14ebe040354/components/password_manager/core/browser/form_parsing/form_parser_unittest.cc [modify] https://crrev.com/eeeaa95d986b05dad65fc40e803fc14ebe040354/components/password_manager/core/browser/form_parsing/password_field_prediction.cc [modify] https://crrev.com/eeeaa95d986b05dad65fc40e803fc14ebe040354/components/password_manager/core/browser/form_parsing/password_field_prediction_unittest.cc
,
Nov 16
The proposal 1) from the issue description is now implemented.
,
Nov 29
vabr going hobby only -> reducing involvement. Please contact me directly in urgent matters. |
||
►
Sign in to add a comment |
||
Comment 1 by dvadym@chromium.org
, Nov 7