Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix wrong autocomplete=new-password on the current password field. #16558

Merged
merged 1 commit into from
Aug 14, 2024
Merged

Fix wrong autocomplete=new-password on the current password field. #16558

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions src/OrchardCore.Modules/OrchardCore.Users/Views/Account/ChangePassword.cshtml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 />
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be kept.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What if you leave your PC unlocked accidentally, and someone changes your password since the current password is auto loaded?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

@gvkries gvkries Aug 14, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 🤷‍♂️

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 😊

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll merge it, then.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why you think it's a bad user experience?

Because it may also prevent password managers from filling in the current password...

<span asp-validation-for="CurrentPassword" class="text-danger"></span>
</div>
</div>
Expand All @@ -26,7 +26,7 @@
<div class="mb-3">
<label asp-for="PasswordConfirmation" class="col-md-4 form-label">@T["New password confirmation"]</label>
<div class="col-md-8">
<input asp-for="PasswordConfirmation" class="form-control" />
<input asp-for="PasswordConfirmation" class="form-control" autocomplete="new-password" />
<span asp-validation-for="PasswordConfirmation" class="text-danger"></span>
</div>
</div>
Expand Down