Skip to content

Set autocomplete property on login form fields #22023

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

Jump to bottom
Merged
merged 2 commits into from
Apr 16, 2024
Merged

Set autocomplete property on login form fields #22023

merged 2 commits into from
Apr 16, 2024

Conversation

bx80
Copy link
Contributor

@bx80 bx80 commented Mar 15, 2024 •
edited by sgiehl
Loading

Description:

This PR sets autocomplete='current-password' for the password field and autocomplete='username' for the userlogin field on the main Matomo login page as the missing property sometime gets flagged as a security risk by security software.

Based on this this comment from a previous issue we should have autocomplete property set on all password fields.

Fixes #22020

Review

@bx80 bx80 added the Bug For errors / faults / flaws / inconsistencies etc. label Mar 15, 2024
@bx80 bx80 self-assigned this Mar 15, 2024
@bx80 bx80 added the Needs Review PRs that need a code review label Mar 15, 2024
@michalkleiner
Copy link
Contributor

My only concern would be password managers. If they still work as expected with this change then I'm happy to merge this. If this prevents autofill by password managers then I would rather document this as an exception.

@michalkleiner michalkleiner requested a review from a team March 16, 2024 06:59
@michalkleiner michalkleiner added this to the 5.1.0 milestone Mar 16, 2024
@bx80
Copy link
Contributor Author

bx80 commented Mar 18, 2024

My only concern would be password managers. If they still work as expected with this change then I'm happy to merge this. If this prevents autofill by password managers then I would rather document this as an exception.

Absolutely, either outcome would be good 👍
Based on older tickets I suspect this issue might have had some prior discussion, maybe @sgiehl can shed some light on any previous decisions? 😃

@sgiehl
Copy link
Member

sgiehl commented Mar 25, 2024

Not sure, why the autocomplete attribute is missing on the login form. I was pretty sure that this would already be the case.
Anyway, looking at other big sites like GitHub, Wikipedia or Google it seems like they are setting autocomplete='current-password' and autocomplete='username' on the login forms. So maybe we should go with this as well?

@github-actions GitHub Actions
Copy link
Contributor

github-actions bot commented Apr 2, 2024

This issue is in "needs review" but there has been no activity for 7 days. ping @matomo-org/core-reviewers

@github-actions github-actions bot added the Stale The label used by the Close Stale Issues action label Apr 2, 2024
@sgiehl sgiehl unassigned bx80 Apr 15, 2024
@sgiehl sgiehl removed this from the 5.1.0 milestone Apr 15, 2024
@sgiehl sgiehl changed the title Set autocomplete=off on the login password field Set autocomplete property on login form fields Apr 15, 2024
@sgiehl sgiehl force-pushed the m22020-login-disable-passwd-autocomplete branch from 46ad1a7 to 6cc98dd Compare April 15, 2024 13:11
@sgiehl sgiehl added this to the 5.2.0 milestone Apr 15, 2024
@sgiehl sgiehl removed the Stale The label used by the Close Stale Issues action label Apr 15, 2024
@sgiehl sgiehl force-pushed the m22020-login-disable-passwd-autocomplete branch from 6cc98dd to 11131b6 Compare April 15, 2024 13:14
@sgiehl sgiehl modified the milestones: 5.2.0, 5.1.0 Apr 16, 2024
@sgiehl sgiehl merged commit 2109156 into 5.x-dev Apr 16, 2024
@sgiehl sgiehl deleted the m22020-login-disable-passwd-autocomplete branch April 16, 2024 08:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@michalkleiner michalkleiner michalkleiner approved these changes

Assignees
No one assigned
Labels
Bug For errors / faults / flaws / inconsistencies etc. Needs Review PRs that need a code review
Milestone
5.1.0
Development

Successfully merging this pull request may close these issues.

[Bug] Password field is set to auto-complete
3 participants
@bx80 @michalkleiner @sgiehl