Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2740523002:
[Password Manager] Fix saving for accounts.google.com. (Closed)
|
Created:
3 years, 9 months ago by dvadym Modified:
3 years, 9 months ago CC:
chromium-reviews, gcasto+watchlist_chromium.org, alemate+watch_chromium.org, achuith+watch_chromium.org, vabr+watchlistpasswordmanager_chromium.org, vabr+watchlistlogin_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Description[Password Manager] Fix saving for accounts.google.com.
Saving on accounts.google.com doesn't work, the reasons are the following:
The sign-in form is loaded on https://accounts.google.com/ServiceLogin, but then when the submission is done, url is https://accounts.google.com/signin/v2/challenge/pwd , so loaded form and the submitted form are considered as different forms, and the Password Manager thinks that the submitted form was not seen during adding to DOM, and such forms are skipped since Password Manager can't fill forms that can't be seen (and no point to save password, that can't be filled). But this is legitimate site behaviour
This CL implements checking whether loaded and submitted forms come from the same frame (a frame is represented with a driver in Password Manager code).
BUG=699097
Review-Url: https://codereview.chromium.org/2740523002
Cr-Commit-Position: refs/heads/master@{#456674}
Committed: https://chromium.googlesource.com/chromium/src/+/411333039edf0a1cfa7ecd505a979af3015545f2
Patch Set 1 #Patch Set 2 : Added tests, pretifying #
Total comments: 4
Patch Set 3 : Remove commented line #
Total comments: 7
Patch Set 4 : Renamed flag #Patch Set 5 : test fixed #Patch Set 6 : compilation fix #
Total comments: 10
Patch Set 7 : Addressed comments #
Total comments: 4
Patch Set 8 : Small coments fixes #
Messages
Total messages: 33 (17 generated)
Description was changed from ========== gmail fix BUG=None ========== to ========== Saving on accounts.google.com doesn't work, the reasons are the following: The sign-in form is loaded on https://accounts.google.com/ServiceLogin, but then when the submission is done, url is https://accounts.google.com/signin/v2/challenge/pwd , so loaded form and the submitted form are considered as different forms, and the Password Manager thinks that the submitted form was not seen during adding to DOM, and such forms are skipped since Password Manager can't fill forms that can't be seen (and no point to save password, that can't be filled). But this is legitimate site behaviour This CL implements checking whether loaded and submitted forms come from the same frame (frame is represented with driver in Password Manager code). BUG=699097 ==========
dvadym@chromium.org changed reviewers: + vasilii@chromium.org
Description was changed from ========== Saving on accounts.google.com doesn't work, the reasons are the following: The sign-in form is loaded on https://accounts.google.com/ServiceLogin, but then when the submission is done, url is https://accounts.google.com/signin/v2/challenge/pwd , so loaded form and the submitted form are considered as different forms, and the Password Manager thinks that the submitted form was not seen during adding to DOM, and such forms are skipped since Password Manager can't fill forms that can't be seen (and no point to save password, that can't be filled). But this is legitimate site behaviour This CL implements checking whether loaded and submitted forms come from the same frame (frame is represented with driver in Password Manager code). BUG=699097 ========== to ========== Saving on accounts.google.com doesn't work, the reasons are the following: The sign-in form is loaded on https://accounts.google.com/ServiceLogin, but then when the submission is done, url is https://accounts.google.com/signin/v2/challenge/pwd , so loaded form and the submitted form are considered as different forms, and the Password Manager thinks that the submitted form was not seen during adding to DOM, and such forms are skipped since Password Manager can't fill forms that can't be seen (and no point to save password, that can't be filled). But this is legitimate site behaviour This CL implements checking whether loaded and submitted forms come from the same frame (a frame is represented with a driver in Password Manager code). BUG=699097 ==========
Hi Vasilii, Could you please review this CL? Regards, Vadym
Description was changed from ========== Saving on accounts.google.com doesn't work, the reasons are the following: The sign-in form is loaded on https://accounts.google.com/ServiceLogin, but then when the submission is done, url is https://accounts.google.com/signin/v2/challenge/pwd , so loaded form and the submitted form are considered as different forms, and the Password Manager thinks that the submitted form was not seen during adding to DOM, and such forms are skipped since Password Manager can't fill forms that can't be seen (and no point to save password, that can't be filled). But this is legitimate site behaviour This CL implements checking whether loaded and submitted forms come from the same frame (a frame is represented with a driver in Password Manager code). BUG=699097 ========== to ========== [Password Manager] Fix saving for accounts.google.com. Saving on accounts.google.com doesn't work, the reasons are the following: The sign-in form is loaded on https://accounts.google.com/ServiceLogin, but then when the submission is done, url is https://accounts.google.com/signin/v2/challenge/pwd , so loaded form and the submitted form are considered as different forms, and the Password Manager thinks that the submitted form was not seen during adding to DOM, and such forms are skipped since Password Manager can't fill forms that can't be seen (and no point to save password, that can't be filled). But this is legitimate site behaviour This CL implements checking whether loaded and submitted forms come from the same frame (a frame is represented with a driver in Password Manager code). BUG=699097 ==========
Can you clarify the scenario? The second form lives in another iFrame and it's submitted? https://codereview.chromium.org/2740523002/diff/20001/components/password_man... File components/password_manager/core/browser/password_form_manager.cc (right): https://codereview.chromium.org/2740523002/diff/20001/components/password_man... components/password_manager/core/browser/password_form_manager.cc:334: }); I'm confused by the fix. PasswordFormManager used to control the same form across all the iFrames. Now it's gonna control in addition all the forms in those iFrames? What happens if another PFM already controls a form there? https://codereview.chromium.org/2740523002/diff/20001/components/password_man... components/password_manager/core/browser/password_form_manager.cc:387: // DCHECK_NE(RESULT_NO_MATCH, DoesManage(credentials)); ??
No second form, it's just the same <form>, but origin of iframe was changed from load. https://codereview.chromium.org/2740523002/diff/20001/components/password_man... File components/password_manager/core/browser/password_form_manager.cc (right): https://codereview.chromium.org/2740523002/diff/20001/components/password_man... components/password_manager/core/browser/password_form_manager.cc:334: }); On 2017/03/07 16:09:33, vasilii wrote: > I'm confused by the fix. PasswordFormManager used to control the same form > across all the iFrames. Now it's gonna control in addition all the forms in > those iFrames? > What happens if another PFM already controls a form there? PasswordForm.origin is determined by the frame origin, but the frame origin could change, and these lines adding processing of this case (because drivers are one-to-one correspondence to frames). These lines are in no way addding control to another forms on frames. https://codereview.chromium.org/2740523002/diff/20001/components/password_man... components/password_manager/core/browser/password_form_manager.cc:387: // DCHECK_NE(RESULT_NO_MATCH, DoesManage(credentials)); On 2017/03/07 16:09:33, vasilii wrote: > ?? Removed, I don't think that it makes sense to pass Driver here, only for DCHECK
A couple of questions below. I'm still struggling to understand what DoesManage really means. Maybe relaxing the comparison between URLs by dropping the path will just work? https://codereview.chromium.org/2740523002/diff/40001/components/password_man... File components/password_manager/core/browser/password_form_manager_unittest.cc (right): https://codereview.chromium.org/2740523002/diff/40001/components/password_man... components/password_manager/core/browser/password_form_manager_unittest.cc:2889: TEST_F(PasswordFormManagerTest, DoesManageDifferentSignonRealmSameDrivers) { How is it possible that one driver has two forms with different signon_realms? If they do history.pushState() then it can work only on the same origin. https://codereview.chromium.org/2740523002/diff/40001/components/password_man... File components/password_manager/core/browser/password_manager.cc (right): https://codereview.chromium.org/2740523002/diff/40001/components/password_man... components/password_manager/core/browser/password_manager.cc:301: (*iter)->DoesManage(form, driver); Is it true that it's the only place where it matters? If the answer is "yes", does it make sense to write here something like this: (*iter)->DoesManage(form) || driver == (*iter)->driver() ? https://codereview.chromium.org/2740523002/diff/40001/components/password_man... components/password_manager/core/browser/password_manager.cc:518: if (old_manager->DoesManage(*iter, driver) != Do I understand right that passing |driver| here isn't necessary to fix the bug?
DoesManage(PasswordForm form) tries to check that |form| is equal |observed_form|. The problems are that PasswordForm is just some extract from real <form> and that JavaScript can change something from the load (i.e. from |observed_form|). It's used for 2 reasons: 1.OnSubmission - to be sure that we saw the submitted form on load (if we didn't see it on load, we can't fill it, so it doesn't make sense to save it). 2.On receiving a new form from the renderer, to check whether we already created PFM and not to create new PFM. This is quite important, because for purposes of robustness renderer sometimes sends the same form multiple times. This explanation how it works now. I think that the current implementation is bad and should be refactored. We will do it in future, but now we should fix the bug :). Dropping the path (it's the same as a comparison by signon_realm) is worse, because it can make that forms from different iframes would match. That's not bad for saving, but it will break sending votes for password generation. The solution in this CL doesn't change form matching rules at all. https://codereview.chromium.org/2740523002/diff/40001/components/password_man... File components/password_manager/core/browser/password_form_manager_unittest.cc (right): https://codereview.chromium.org/2740523002/diff/40001/components/password_man... components/password_manager/core/browser/password_form_manager_unittest.cc:2889: TEST_F(PasswordFormManagerTest, DoesManageDifferentSignonRealmSameDrivers) { On 2017/03/08 17:18:49, vasilii wrote: > How is it possible that one driver has two forms with different signon_realms? > If they do history.pushState() then it can work only on the same origin. It's impossible for normal flow, but I've added this condition in PFM.DoesManage, to be on safe side that no credentials are stored on different signon_realm, so here I've added a test for this. Do you think checking |signon_realm| is too paranoic? https://codereview.chromium.org/2740523002/diff/40001/components/password_man... File components/password_manager/core/browser/password_manager.cc (right): https://codereview.chromium.org/2740523002/diff/40001/components/password_man... components/password_manager/core/browser/password_manager.cc:301: (*iter)->DoesManage(form, driver); On 2017/03/08 17:18:50, vasilii wrote: > Is it true that it's the only place where it matters? > > If the answer is "yes", does it make sense to write here something like this: > (*iter)->DoesManage(form) || driver == (*iter)->driver() > > ? This is the place where it's critical. DoesManage checks not only by origin, so it's not equavalent and we can take incorrect PFM in case when there are more than 1 forms in one frame. https://codereview.chromium.org/2740523002/diff/40001/components/password_man... components/password_manager/core/browser/password_manager.cc:518: if (old_manager->DoesManage(*iter, driver) != On 2017/03/08 17:18:50, vasilii wrote: > Do I understand right that passing |driver| here isn't necessary to fix the bug? Not critical, but otherwise extra PFM could be created.
I see. I am still uncomfortable with calling the PFM origin matching when it's not. Can you introduce a new flag (RESULT_MATCH_DRIVER) to MatchResultFlags and use it where appropriate? https://codereview.chromium.org/2740523002/diff/40001/components/password_man... File components/password_manager/core/browser/password_form_manager_unittest.cc (right): https://codereview.chromium.org/2740523002/diff/40001/components/password_man... components/password_manager/core/browser/password_form_manager_unittest.cc:2889: TEST_F(PasswordFormManagerTest, DoesManageDifferentSignonRealmSameDrivers) { On 2017/03/09 12:42:02, dvadym wrote: > On 2017/03/08 17:18:49, vasilii wrote: > > How is it possible that one driver has two forms with different signon_realms? > > If they do history.pushState() then it can work only on the same origin. > > It's impossible for normal flow, but I've added this condition in > PFM.DoesManage, to be on safe side that no credentials are stored on different > signon_realm, so here I've added a test for this. Do you think checking > |signon_realm| is too paranoic? Acknowledged.
lgtm
The CQ bit was checked by dvadym@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_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by dvadym@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...
dvadym@chromium.org changed reviewers: + vabr@chromium.org
Hi Vaclav, could you please review changes in chrome/browser/ui/login/login_handler.cc ? Regards, Vadym
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
LGTM with comments. Thanks for fixing the issue! Vaclav https://codereview.chromium.org/2740523002/diff/100001/components/password_ma... File components/password_manager/core/browser/password_form_manager.cc (left): https://codereview.chromium.org/2740523002/diff/100001/components/password_ma... components/password_manager/core/browser/password_form_manager.cc:375: DCHECK_NE(RESULT_NO_MATCH, DoesManage(credentials)); Why does this condition no longer hold? https://codereview.chromium.org/2740523002/diff/100001/components/password_ma... File components/password_manager/core/browser/password_form_manager.h (right): https://codereview.chromium.org/2740523002/diff/100001/components/password_ma... components/password_manager/core/browser/password_form_manager.h:90: // Compares basic data of |observed_form_| with |form| and returns how much Please update the comment to describe what |driver| is for. https://codereview.chromium.org/2740523002/diff/100001/components/password_ma... File components/password_manager/core/browser/password_form_manager_unittest.cc (right): https://codereview.chromium.org/2740523002/diff/100001/components/password_ma... components/password_manager/core/browser/password_form_manager_unittest.cc:2877: EXPECT_NE(observed_form()->origin, submitted_form.origin); optional: Please consider re-setting observed_form()->origin to a literal different from line 2876 instead of just checking that it is indeed different. EXPECT* should describe expectations about the tested code, not about the test fixture set-up. If someone changes the default values, they should not need to go through all the tests and think hard about why expectations are failing there. https://codereview.chromium.org/2740523002/diff/100001/components/password_ma... File components/password_manager/core/browser/password_manager.h (right): https://codereview.chromium.org/2740523002/diff/100001/components/password_ma... components/password_manager/core/browser/password_manager.h:131: void ProvisionallySavePassword( Also here, please explain |driver| in the comment.
The CQ bit was checked by dvadym@chromium.org to run a CQ dry run
Thanks for review Vaclav! https://codereview.chromium.org/2740523002/diff/100001/components/password_ma... File components/password_manager/core/browser/password_form_manager.cc (left): https://codereview.chromium.org/2740523002/diff/100001/components/password_ma... components/password_manager/core/browser/password_form_manager.cc:375: DCHECK_NE(RESULT_NO_MATCH, DoesManage(credentials)); On 2017/03/13 17:36:28, vabr (Chromium) wrote: > Why does this condition no longer hold? DoesManage() has now a driver as a parameter. My feeling that it's not correct to pass an additional parameter to ProvisionallySave only for DCHECK https://codereview.chromium.org/2740523002/diff/100001/components/password_ma... File components/password_manager/core/browser/password_form_manager.h (right): https://codereview.chromium.org/2740523002/diff/100001/components/password_ma... components/password_manager/core/browser/password_form_manager.h:90: // Compares basic data of |observed_form_| with |form| and returns how much On 2017/03/13 17:36:28, vabr (Chromium) wrote: > Please update the comment to describe what |driver| is for. Done. https://codereview.chromium.org/2740523002/diff/100001/components/password_ma... File components/password_manager/core/browser/password_form_manager_unittest.cc (right): https://codereview.chromium.org/2740523002/diff/100001/components/password_ma... components/password_manager/core/browser/password_form_manager_unittest.cc:2877: EXPECT_NE(observed_form()->origin, submitted_form.origin); On 2017/03/13 17:36:28, vabr (Chromium) wrote: > optional: Please consider re-setting observed_form()->origin to a literal > different from line 2876 instead of just checking that it is indeed different. > > EXPECT* should describe expectations about the tested code, not about the test > fixture set-up. If someone changes the default values, they should not need to > go through all the tests and think hard about why expectations are failing > there. Done. https://codereview.chromium.org/2740523002/diff/100001/components/password_ma... File components/password_manager/core/browser/password_manager.h (right): https://codereview.chromium.org/2740523002/diff/100001/components/password_ma... components/password_manager/core/browser/password_manager.h:131: void ProvisionallySavePassword( On 2017/03/13 17:36:28, vabr (Chromium) wrote: > Also here, please explain |driver| in the comment. Done.
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Still some minor nits, still LGTM. :) https://codereview.chromium.org/2740523002/diff/100001/components/password_ma... File components/password_manager/core/browser/password_form_manager.cc (left): https://codereview.chromium.org/2740523002/diff/100001/components/password_ma... components/password_manager/core/browser/password_form_manager.cc:375: DCHECK_NE(RESULT_NO_MATCH, DoesManage(credentials)); On 2017/03/14 09:56:35, dvadym wrote: > On 2017/03/13 17:36:28, vabr (Chromium) wrote: > > Why does this condition no longer hold? > > DoesManage() has now a driver as a parameter. My feeling that it's not correct > to pass an additional parameter to ProvisionallySave only for DCHECK Could we just call it with nullptr? IIUC, the driver is important once the landing site is loaded, correct? Otherwise I'm OK with deleting the DCHECK, I agree that we should not need to pass the driver just for that. https://codereview.chromium.org/2740523002/diff/120001/components/password_ma... File components/password_manager/core/browser/password_form_manager.h (right): https://codereview.chromium.org/2740523002/diff/120001/components/password_ma... components/password_manager/core/browser/password_form_manager.h:93: // comes from. Bu apparently it also can be null. Could you explain the meaning of null here as well? https://codereview.chromium.org/2740523002/diff/120001/components/password_ma... File components/password_manager/core/browser/password_manager.h (right): https://codereview.chromium.org/2740523002/diff/120001/components/password_ma... components/password_manager/core/browser/password_manager.h:132: // comes from. ditto about explaining the special null value here.
https://codereview.chromium.org/2740523002/diff/100001/components/password_ma... File components/password_manager/core/browser/password_form_manager.cc (left): https://codereview.chromium.org/2740523002/diff/100001/components/password_ma... components/password_manager/core/browser/password_form_manager.cc:375: DCHECK_NE(RESULT_NO_MATCH, DoesManage(credentials)); On 2017/03/14 10:31:57, vabr (Chromium) wrote: > On 2017/03/14 09:56:35, dvadym wrote: > > On 2017/03/13 17:36:28, vabr (Chromium) wrote: > > > Why does this condition no longer hold? > > > > DoesManage() has now a driver as a parameter. My feeling that it's not correct > > to pass an additional parameter to ProvisionallySave only for DCHECK > > Could we just call it with nullptr? IIUC, the driver is important once the > landing site is loaded, correct? > > Otherwise I'm OK with deleting the DCHECK, I agree that we should not need to > pass the driver just for that. Yes, the most important case when a landing page is loaded. We could pass nullptr, but the result might be different :). https://codereview.chromium.org/2740523002/diff/120001/components/password_ma... File components/password_manager/core/browser/password_form_manager.h (right): https://codereview.chromium.org/2740523002/diff/120001/components/password_ma... components/password_manager/core/browser/password_form_manager.h:93: // comes from. On 2017/03/14 10:31:57, vabr (Chromium) wrote: > Bu apparently it also can be null. Could you explain the meaning of null here as > well? Done. https://codereview.chromium.org/2740523002/diff/120001/components/password_ma... File components/password_manager/core/browser/password_manager.h (right): https://codereview.chromium.org/2740523002/diff/120001/components/password_ma... components/password_manager/core/browser/password_manager.h:132: // comes from. On 2017/03/14 10:31:57, vabr (Chromium) wrote: > ditto about explaining the special null value here. Done.
The CQ bit was checked by dvadym@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from vasilii@chromium.org, vabr@chromium.org Link to the patchset: https://codereview.chromium.org/2740523002/#ps140001 (title: "Small coments fixes")
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": 140001, "attempt_start_ts": 1489491003870140,
"parent_rev": "5a5328db5be00d556034c92224f05904e863b410", "commit_rev":
"411333039edf0a1cfa7ecd505a979af3015545f2"}
Message was sent while issue was closed.
Description was changed from ========== [Password Manager] Fix saving for accounts.google.com. Saving on accounts.google.com doesn't work, the reasons are the following: The sign-in form is loaded on https://accounts.google.com/ServiceLogin, but then when the submission is done, url is https://accounts.google.com/signin/v2/challenge/pwd , so loaded form and the submitted form are considered as different forms, and the Password Manager thinks that the submitted form was not seen during adding to DOM, and such forms are skipped since Password Manager can't fill forms that can't be seen (and no point to save password, that can't be filled). But this is legitimate site behaviour This CL implements checking whether loaded and submitted forms come from the same frame (a frame is represented with a driver in Password Manager code). BUG=699097 ========== to ========== [Password Manager] Fix saving for accounts.google.com. Saving on accounts.google.com doesn't work, the reasons are the following: The sign-in form is loaded on https://accounts.google.com/ServiceLogin, but then when the submission is done, url is https://accounts.google.com/signin/v2/challenge/pwd , so loaded form and the submitted form are considered as different forms, and the Password Manager thinks that the submitted form was not seen during adding to DOM, and such forms are skipped since Password Manager can't fill forms that can't be seen (and no point to save password, that can't be filled). But this is legitimate site behaviour This CL implements checking whether loaded and submitted forms come from the same frame (a frame is represented with a driver in Password Manager code). BUG=699097 Review-Url: https://codereview.chromium.org/2740523002 Cr-Commit-Position: refs/heads/master@{#456674} Committed: https://chromium.googlesource.com/chromium/src/+/411333039edf0a1cfa7ecd505a97... ==========
Message was sent while issue was closed.
Committed patchset #8 (id:140001) as https://chromium.googlesource.com/chromium/src/+/411333039edf0a1cfa7ecd505a97... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
