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.

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

Already on GitHub? Sign in to your account

Apply autocomplete attribute consistently to all forms #10604

Merged
merged 4 commits into from May 13, 2024

Conversation

aduth
Copy link
Member

@aduth aduth commented May 10, 2024

馃洜 Summary of changes

Monkey-patches SimpleForm to apply the autocomplete="off" attribute automatically for all forms.

Why?

  • Reduce boilerplate for form markup
  • Fix inconsistencies (see instances below in "Audit" for forms where we should have this attribute, but it is not currently applied)

Audit:

I reviewed every simple_form_for as part of this work. Aside from those modified in the included changes, the views below included simple_form_for without an explicit autocomplete, along with a rationale for why it should be opted in to automatic autocomplete with these changes:

  • app/views/idv/otp_verification/show.html.erb
    • OneTimeCodeInputComponent applies its own autocomplete
  • app/views/idv/shared/_document_capture.html.erb
    • Only hidden fields
  • app/views/saml_idp/shared/saml_post_binding.html.erb
    • Only hidden fields
  • app/views/shared/_personal_key.html.erb
    • Personal key should not be autocompleted
  • app/views/shared/saml_post_form.html.erb
    • Only hidden fields
  • app/views/sign_up/completions/show.html.erb
    • Only includes button
  • app/views/sign_up/emails/show.html.erb
    • Only includes hidden fields and button
  • app/views/test/piv_cac_authentication_test_subject/new.html.erb
    • Test route
  • app/views/two_factor_authentication/otp_verification/show.html.erb
    • OneTimeCodeInputComponent applies its own autocomplete
  • app/views/two_factor_authentication/totp_verification/show.html.erb
    • OneTimeCodeInputComponent applies its own autocomplete
    • Otherwise includes only checkbox and button
  • app/views/two_factor_authentication/webauthn_verification/show.html.erb
    • Only includes checkbox and button
  • app/views/users/backup_code_setup/index.html.erb
    • Only includes checkbox and button
  • app/views/users/email_language/show.html.erb
    • Only includes radio options and button
  • app/views/users/piv_cac_authentication_setup/new.html.erb
    • This has a text field for nickname, but reasonable expectation we would not want to support autocomplete
  • app/views/users/piv_cac_setup_from_sign_in/prompt.html.erb
    • This has a text field for nickname, but reasonable expectation we would not want to support autocomplete
  • app/views/users/totp_setup/new.html.erb
    • This has a text field for nickname, but reasonable expectation we would not want to support autocomplete
  • app/views/users/two_factor_authentication_setup/index.html.erb
    • Only includes checkboxes and button
  • app/views/users/verify_personal_key/new.html.erb
    • Personal key should not be autocompleted

馃摐 Testing Plan

Verify no regressions in form autocompletion.

changelog: Bug Fixes, Forms, Disable autocomplete consistently for all forms
@@ -11,7 +11,6 @@
<%= simple_form_for(
@password_reset_email_form,
url: user_password_path,
html: { autocomplete: 'off', method: :post },
Copy link
Member Author

Choose a reason for hiding this comment

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

method: :post is default, so removed it.

Copy link
Member Author

Choose a reason for hiding this comment

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

method: :post is default, so removed it.

I don't think it's actually so straight-forward, there may be cases where we want explicit method: :post. Unclear which of these are candidates. Could be safer to back out the changes.

https://github.com/rails/rails/blob/6f0d1ad14b92b9f5906e44740fce8b4f1c7075dc/actionview/lib/action_view/helpers/form_helper.rb#L1595

Copy link
Member Author

Choose a reason for hiding this comment

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

Added them back in 7dead6b for simple_form_for where the record is a persisted model (e.g. current_user).

config/initializers/simple_form.rb Outdated Show resolved Hide resolved
@aduth aduth merged commit c211ccf into main May 13, 2024
2 checks passed
@aduth aduth deleted the aduth-autocomplete-all-forms branch May 13, 2024 12:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants