(There is no prompt asking the user whether they'd like to update the stored password.)

Thanks very much!

While I do have a DKB account, the registration form seems to involve a postal step between, so I'll try to deduce what happened from the logs and build a non-banking repro case.

The DKB login form is at https://www.dkb.de, while the card registration form is at https://secure.dkb.de. Those two origins are a "Public Suffix List-match" -- their scheme and port match and the e-TLD+1 of their hostname (dkb.de) match.

If you have a login stored on one origin, and then go to a PSL-matching different origin, Chrome can fill the form from the former origin for you. If you submit it as such, it will save the data on the new origin for you, without asking. The rationale is that PSL-matching origins are owned by the same entity, so the user's consent to save for the first one carries over to the second one. This is to maximise convenience when Chrome can be fairly sure no security is compromised.

This does not entirely explain your case, because you did not save the same password value, you did save a new one, and that one replaced the old one. But I do see "wait_for_username: true" in the logs, which indicates that Chrome did not auto-fill but allowed to fill manually, which is a sign of filling a PSL-matching credential.

The other clues I see in the logs are that the credit card registration form does not seem to have a username. Also, it has two password fields. While those mean "new PIN" and "confirm PIN", Chrome initially understands it as a change password form ("old PIN", "new PIN"). It might be that the combination of the three things together (PSL-matching with saving without consent, absence of username making it impossible to indicate that this is not the same credential as on dkb.de, and the change password form facilitating the update) enables the bug we are seeing.

I will try to reconstruct this situation either in a browsertest or on a test page to verify, and then think about a fix.

Still no repro case, but I think I know the cause now.

Chrome tries its best to update stored credentials even in cases when the, say, password reset form has no username. Imagine that the user has credentials for user@example.org. They log into their account and choose to change their password. The site offers a form which only contains the new password field, because, say, the user just logged in and hence it's clear that it's them. Now Chrome sees just the new password submitted and wonders what to do with it. Should it save it as a new username-less entry? Or perhaps update user/oldpassword to user/newpassword?

The code at [1] handles just that. And the decision [2] is that as long as the user only has one saved pair of credentials, Chrome should rewrite that with user/newpassword.

This is meant to work for exact (non-PSL) matches. In that case, the "Update?" prompt is shown and the user can decline. Unfortunately, currently also PSL-matched credentials are considered here, and those lead to skipping the prompt [3].

I think the fix is to exclude PSL-matched credentials from the above considerations. I will still need to work on a reproduction case to be sure.



[1] https://chromium.googlesource.com/chromium/src/+/3df24d27fb05805b8e15b8515bfbf12a9bd8c29c/components/password_manager/core/browser/password_form_manager.cc#716

[2] https://chromium.googlesource.com/chromium/src/+/3df24d27fb05805b8e15b8515bfbf12a9bd8c29c/components/password_manager/core/browser/password_form_manager.cc#795

[3] https://chromium.googlesource.com/chromium/src/+/3df24d27fb05805b8e15b8515bfbf12a9bd8c29c/components/password_manager/core/browser/password_manager.cc#780

I have a repro case:

(1) Go to http://1.chromium-test1.appspot.com/testing/psl-matching/login, save username/oldpassword.
(2) Go to http://2.chromium-test1.appspot.com/testing/psl-matching/login, use DevTools to replace the username and password fields with this:

<li>
<input type="password" id="password2" name="password2" autocomplete="new-password" aria-autocomplete="list">
</li>
<li>
<input type="password" id="password" name="password" autocomplete="new-password">
</li>

(3) Fill "newpassword" in both fields, hit Submit.

The result are silently rewritten credentials, just as in the dkb.de case.


My next step is a browser test exhibiting this. Then a fix for PasswordFormManager and also for NewPasswordFormManager, if needed.

(Cc-ed Vadym in case this is interesting to him, e.g., because of NewPasswordFormManager.)

The test and fix are in https://crrev.com/c/1114740.
There is no work needed for NewPasswordFormManager, because that has not implemented saving yet.

This is a nasty bug (thanks, tnagel@, for reporting it and providing the useful details!), I'll try to get it merged as far as I can.

Thanks a lot, Vaclav! Regarding comment #3, the fix that you're proposing solves the problem based on the implementation detail (by DKB) that the credit card PIN form and the main login form are on two different subdomains. This doesn't need to be the case, it would be totally reasonable to have both forms on the same subdomain and I would hope that we could address this as well.

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/bd80614ff8e959bc736b76c024cf2059367e07ac

commit bd80614ff8e959bc736b76c024cf2059367e07ac
Author: Vaclav Brozek <vabr@chromium.org>
Date: Tue Jun 26 20:54:10 2018

Exclude PSL-matched forms from update candidates

If Chrome sees a username-less form being submitted on an origin, and
has exactly one pair of credentials stored for that origin, it offers
the user to update that pair with the new password, as a convenience.

However, if the only credential is stored for a PSL-related (having the
same eTLD+1) but different origin, this update should not be offered.
There is much less chance that the old credential and the new password
actually represent the same account.

Also, saving PSL-related credentials has the side effect of skipping the
confirmation prompt, so such update happens without user confirmation.
That's bad, because it led to overwriting useful credentials (see the
attached bug).

Therefore, this CL removes PSL-matching credentials from the list of
candidates to be updated.

Bug:  856543 
Change-Id: Ibb77eaf5bf22d5865b83edd0bbd1bc6e02c41107
Reviewed-on: https://chromium-review.googlesource.com/1114740
Commit-Queue: Vaclav Brozek <vabr@chromium.org>
Reviewed-by: Dominic Battré <battre@chromium.org>
Cr-Commit-Position: refs/heads/master@{#570525}
[modify] https://crrev.com/bd80614ff8e959bc736b76c024cf2059367e07ac/chrome/browser/password_manager/password_manager_browsertest.cc
[add] https://crrev.com/bd80614ff8e959bc736b76c024cf2059367e07ac/chrome/test/data/password/new_password_form.html
[modify] https://crrev.com/bd80614ff8e959bc736b76c024cf2059367e07ac/components/password_manager/core/browser/password_form_manager.cc

Hi Vaclav and Vadym,

thanks for looking into this and for moving quickly. This is an interesting case which does not look like a bug fix to me :)

I would like to chat about the implied product behavior changes here. I am not sure I understand how this change covers cases where sites might offer different types of "passwords" (e.g. classic passwords and PINs - both set via type=password fields). And I am not sure about the impact of removing PSL-matching support for change password forms. This is something we should discuss. I'll set up time.

Cheers

Patrick

An update after the conversation with Patrick:

Patrick raised a concern that apart from fixing the reported bug, the change r570525 also changes Chrome's behaviour in cases when the silent update was lucky and did what the user wanted. Those are situations, where:
 * a page has all forms which Chrome could fill (e.g., login forms) on one subdomain -- login.page.com, and
 * the same page has a password update form without a field for username or the old password on another subdomain -- change.page.com
In those cases, after r570525, the old stored credential is only associated with login.page.com, and cannot be filled anywhere on change.page.com. Therefore, the user has no chance to confirm that it actually represents one logical account across both subdomains (which is done by filling a PSL-matched hint), and hence Chrome will have not guessed that it needs to be updated with the new password. As a result, the user will end up having stored username/oldpassword on login.page.com and <no username>/newpassword on change.page.com. They will have troubles filling the new password on login.page.com -- they can either just type it in again there for Chrome to update the login.page.com entry independently of other subdomains, or they can delete the old credential, get the password filled via PSL matching and retype the username again.

So, effectively, r570525 swapped a data-losing problem in some cases for loss of convenience in other cases.

The question is: what can we do to fix the data-losing, while still making it easier to update across subdomains. The issue with this bug was that it happened without user confirmation, so the general direction would be to ensure that the user is always notified and keep providing the option to update credentials across subdomains. As every increase of complexity in the logic, this might introduce new edge cases, so we need to carefully specify the behaviour in them (what if there are PSL and non-PSL matches, etc.). We also need to make sure that the user understands what credential they are about to change (currently, the Update dialog does not present the domain, because it is always the current one).

It is unclear to me, how large are the affected populations of both the data-loss and the inconvenience problems. On the assumption that their size is similar, the data-loss seems more important to get fixed than the inconvenience. Because r570525 is small and simple, it should be possible to merge it at least into M68. Anything along the lines of the more complex solution described in the previous paragraphs is unlikely to make it into M69 (UX and eng work involved, unclear priority given the unclear number of users impacted, M69 feature freeze in the past).

My own plan is:
 * keep r570525 in M69
 * attempt to merge it to M68 (likely) and M67 (unlikely)

Patrick, if you want me to change the above, or to work more on the bigger solution, let's talk.

A side note:

When I talked to dvadym@ about this issue, he reminded me of a related feature -- keeping the history of passwords saved. This way, the user could undo similarly destructive actions, caused by Chrome or the user, in the future.

The history feature has been discussed before, but never made it to the top of the priority list. If we start to consider investing into work in this area (such as the broader fix described in #10), we might consider how the priority of the history feature compares to that. The number of users affected will very likely be higher for history than for edge-cases like the one described in #10.

Also, I will be holding off with the merge until at least next Tuesday, when I'm meeting dvadym@ for a discussion about better solutions.

I just experienced the same issue. I started with
client.schwab.com username real_password

Then after some manipulations it turned into 
client.schwab.com username real_password
lms.schwab.com username real_password

Then the trick part. I submitted a form with OTP. Now I have 
client.schwab.com username pin
client.schwab.com username real_password
lms.schwab.com username pin

I wasn't asked anything.

This seems even weirder in that client.schwab.com go updated back in the pin (and lms.schwab.com had a copy of the real_password before submitting the OTP.

Can you reproduce repeatedly (and get the internals logs) or was this something which is only a one-off operation at Schwab?

Also, you now have two different passwords stored for the same username at client.schwab.com? Which one is suggested for the login form at client.schwab.com?

This is my Zoo:

              origin_url = https://client.schwab.com/login/signon/customercenterlogin.aspx
              action_url = https://client.schwab.com/login/signon/SignOn.ashx
        username_element = ctl00$WebPartManager1$CenterLogin$LoginUserControlId$txtLoginID
          username_value = *****
        password_element = txtPassword
;kj?B     password_value = v10?????JS?AJ?? // real password
          submit_element = 
            signon_realm = https://client.schwab.com/
               preferred = 1
            date_created = 13050064404000000
     blacklisted_by_user = 0
                  scheme = 0
           password_type = 0
              times_used = 9
               form_data = 
             date_synced = 13137506687372118
            display_name = 
                icon_url = 
          federation_url = 
         skip_zero_click = 0
generation_upload_status = 0
 possible_username_pairs = 

              origin_url = https://lms.schwab.com/Login
              action_url = https://lms.schwab.com/Login
        username_element = LoginId
          username_value = *****
        password_element = Password
          password_value = v10?磊ԍ(~?_??Ϥ // This is OTP
          submit_element = 
            signon_realm = https://lms.schwab.com/
               preferred = 1
            date_created = 13175040009214486
     blacklisted_by_user = 0
                  scheme = 0
           password_type = 0
              times_used = 11
               form_data = $
             date_synced = 13175082006589663
            display_name = 
                icon_url = 
          federation_url = 
         skip_zero_click = 0
generation_upload_status = 0
 possible_username_pairs = 

              origin_url = https://client.schwab.com/login/signon/customercenterlogin.aspx
              action_url = https://ew8client.schwab.com/form.php
        username_element = form_payee_signatory
          username_value = *****
        password_element = password
          password_value = v10?磊ԍ(~?_??Ϥ // This is OTP
          submit_element = 
            signon_realm = https://client.schwab.com/
               preferred = 1
            date_created = 13050064404000000
     blacklisted_by_user = 0
                  scheme = 0
           password_type = 0
              times_used = 10
               form_data = $
             date_synced = 13175082006589663
            display_name = 
                icon_url = 
          federation_url = 
         skip_zero_click = 0
generation_upload_status = 0
 possible_username_pairs =

The wrong OTP is autofilled and there is no way to switch to the correct one except copying it from the settings.
I could try to reproduce it again. Which state do you want me to start in?

Thanks for the additional details, Vasilii.

Actually, let's not bother with reproduction just yet (perhaps later, to confirm a fix). I now realised that in your case the credentials had a username, which is different from the DKB case (and not fixed by my patch above). While the username-less credentials get matched by PasswordFormManager::FindBestMatchForUpdatePassword, the ones with username get matched by PasswordFormManager::FindBestSavedMatch.

Vadym prepares an alternative solution to this issue which will just ensure that the update prompt is always shown (unless coming from CM API or the known issue of a generated password without username), which should address your case as well.

Also, I do think that storing the username/real_password without actually being able to fill it is a bug, just a different one from this one. Jan's intern will work on a tool for fixing such states and I'll make sure that this is considered as a target for that tool. I'd also like to prevent such states from happening, but currently don't have a good idea how.

After another team meeting, the plan is now:

(1) As a hotfix, dvadym@ will replace r570525 with https://crrev.com/c/1122134 on trunk (69). The new fix will not remove update candidates even if they are PSL-matching, but will always ensure that the user can cancel the update via the update prompt. We'll gather some data to verify that this did not break stuff (looking at crashes and likely some added UMA to count the occasions when the patch made a difference), and if all looks good, will request merge to 68. At that point 68 will be in stable already, so nepper@ will help us to coordinate this with TPMs and we'll write a postmortem if the merge happens.

(2) As a mid-term fix, we'll do some work on UI to help users better understand whether they actually want to confirm or cancel the update.

(3) As a long-term fix, we'll look into changing the representation of PSL credential and allowing the user more control over matching credentials across (PSL-related) origins.

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/b1fc7250a7f89e2e94298d90101f2cbed97b7719

commit b1fc7250a7f89e2e94298d90101f2cbed97b7719
Author: Vadym Doroshenko <dvadym@chromium.org>
Date: Fri Jul 13 14:47:36 2018

Always show prompt on update credentials in Password Manager.

There is a bug that Password Manager might silently update credentials
on successful submit of PSL matched form which doesn't have username.
That's bad, because pretty often that's not right thing. The submitted
form can be SMS verification for example.

This CL simplifies PasswordManager::ShouldPromptUserToSavePassword and
makes it returns true in case of password update as result the update
bubble is always shown when on password update.

Bug:  856543 
Change-Id: I7579610e084c456ce22ca568c4f8a63284e63a5a
Reviewed-on: https://chromium-review.googlesource.com/1122134
Commit-Queue: Vadym Doroshenko <dvadym@chromium.org>
Reviewed-by: Ilya Sherman <isherman@chromium.org>
Reviewed-by: Vaclav Brozek <vabr@chromium.org>
Cr-Commit-Position: refs/heads/master@{#574913}
[modify] https://crrev.com/b1fc7250a7f89e2e94298d90101f2cbed97b7719/chrome/browser/password_manager/password_manager_browsertest.cc
[modify] https://crrev.com/b1fc7250a7f89e2e94298d90101f2cbed97b7719/components/password_manager/core/browser/password_form_manager.cc
[modify] https://crrev.com/b1fc7250a7f89e2e94298d90101f2cbed97b7719/components/password_manager/core/browser/password_manager.cc
[modify] https://crrev.com/b1fc7250a7f89e2e94298d90101f2cbed97b7719/components/password_manager/core/browser/password_manager.h
[modify] https://crrev.com/b1fc7250a7f89e2e94298d90101f2cbed97b7719/tools/metrics/histograms/enums.xml
[modify] https://crrev.com/b1fc7250a7f89e2e94298d90101f2cbed97b7719/tools/metrics/histograms/histograms.xml

Thanks, Vadym, for the new fix. I feel it makes sense to hand this over to you, as I expect you will be driving the potential merge to M68. If you disagree, just return the bug to me.

Just for the record, https://docs.google.com/document/d/15A8PazYxWsXwuSM44aVSHvyr3LkpIh8SPpwrpuiK6ZM/edit?usp=sharing describes the situation and the plan we discussed as a team. My apologies for this being restricted to @google.com only, but the doc started as just some quick internal notes and publishing it widely would mean moving it into another doc. Happy to consider giving access individuals on demand (explaining why you need it helps).

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/98f6b13bf4078536223a9559f102f72bdc22857d

commit 98f6b13bf4078536223a9559f102f72bdc22857d
Author: Vaclav Brozek <vabr@chromium.org>
Date: Wed Aug 29 07:04:42 2018

Remove PasswordManager.ShouldShowPromptComparison

As explained in https://crbug.com/871509#c3, the histogram
PasswordManager.ShouldShowPromptComparison was meant for a decision
which is now moot. Therefore, this CL removes the instrumentation for
this histogram and marks the histogram as obsolete.

Bug: 871509,  856543 
Change-Id: Ie46192fc38c8ad699b31519c4dbda60edd82df34
Reviewed-on: https://chromium-review.googlesource.com/1193866
Reviewed-by: Vadym Doroshenko <dvadym@chromium.org>
Reviewed-by: Steven Holte <holte@chromium.org>
Commit-Queue: Vaclav Brozek <vabr@chromium.org>
Cr-Commit-Position: refs/heads/master@{#587028}
[modify] https://crrev.com/98f6b13bf4078536223a9559f102f72bdc22857d/components/password_manager/core/browser/password_manager.cc
[modify] https://crrev.com/98f6b13bf4078536223a9559f102f72bdc22857d/components/password_manager/core/browser/password_manager.h
[modify] https://crrev.com/98f6b13bf4078536223a9559f102f72bdc22857d/tools/metrics/histograms/enums.xml
[modify] https://crrev.com/98f6b13bf4078536223a9559f102f72bdc22857d/tools/metrics/histograms/histograms.xml