Fix wrong autocomplete=new-password on the current password field. #16558
Conversation
|
It should be on both. The idea is to not auto fill the new password or the confirmation of the new password. Adding the |
| @@ -11,7 +11,7 @@ | |||
| <div class="mb-3"> | |||
| <label asp-for="CurrentPassword" class="col-md-4 form-label">@T["Current password"]</label> | |||
| <div class="col-md-8"> | |||
| <input asp-for="CurrentPassword" class="form-control" autofocus autocomplete="new-password" /> | |||
| <input asp-for="CurrentPassword" class="form-control" autofocus /> | |||
There was a problem hiding this comment.
But why? The current password can be autocompleted without a problem IMHO. We could use current-password instead, but I don't know which browser supports that value.
There was a problem hiding this comment.
What if you leave your PC unlocked accidentally, and someone changes your password since the current password is auto loaded?
There was a problem hiding this comment.
These changes are correct IMO. Having autocomplete="new-password" for the current password is simply incorrect (since, well, it's not a new password, it's like the password field on the login screen), and the other two fields are new passwords.
Note that autocomplete="new-password" or the lack of it doesn't affect whether you can insert the current password in your browser from the password manager (at least under latest Chrome), only whether it's already pre-filled on page load. But with two clicks, you can still fill it. This is not a security matter (since if your browser's password manager is unlocked, one can get the password in any case, and if it's locked, one can't in any case again) but a UX one.
There was a problem hiding this comment.
What if you leave your PC unlocked accidentally, and someone changes your password since the current password is auto loaded?
In that case you're already screwed 🙈 That's not what new-password was meant for, autocomplete is a hint for the browser, no security feature 🤷♂️
There was a problem hiding this comment.
yes it's not security thing. But just another step avoid exposing passwords. I suggest keeping new-password. Either way, how often does one user change their password to auto fill it for them.
There was a problem hiding this comment.
I think that is a bad user experience and actually browsers started to ignore autocomplete=off on passwords for the same reason.
So I wouldn't add new-password here, but I let the majority decide 😊
There was a problem hiding this comment.
Why you think it's a bad user experience? Either way, I am okay with the majority direction too. so if you and @Piedone convinced that we should not add new-password, then I am okay with removing it.
There was a problem hiding this comment.
Why you think it's a bad user experience?
Because it may also prevent password managers from filling in the current password...
|
You can now commit directly to the repo @gvkries BTW. It's easier for others to checkout your branch that way. Otherwise, nothing to change, the branch name pattern you use is already good. |
This
autocompletewas supposed to go on the password confirmation, not current password. Correct?/cc @MikeAlhayek @Piedone