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’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Honeypot fields and logic #48

Merged
merged 12 commits into from
Jun 28, 2024
Merged

Conversation

SammySteiner
Copy link
Contributor

Ticket

Changes

  • Added honeypot field to uswds forms helper
  • Added custom honeypot field error message
  • Added honeypot validations to form models
  • Added honeypot field to controllers
  • Added honeypot to publicly accessible forms:
    • sign in
    • create account
    • forgot password
    • reset password
  • Added tests
  • Updated security doc

Context for reviewers

Added honeypot fields to publicly accessible forms to combat spam bots, following guidance from the rails security best practices doc
.

Testing

  • Run tests
  • Tab through the pages and confirm the honeypot fields aren't focused (Ideally with a screen reader)
  • Submit fields normally and confirm normal behavior
  • Edit the hp_field's value to confirm the form fails to submit

@SammySteiner SammySteiner requested a review from rocketnova June 28, 2024 20:45
Copy link
Contributor

@rocketnova rocketnova left a comment

Choose a reason for hiding this comment

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

Thank you for working on this!

For app/controllers and app/forms/users, could we use the variable honeypot instead of hp_field, so that the field naming convention is consistent and the purpose of the field is spelled out? Without knowing in advance, I think it is hard to guess what hp stands for in this context.

app-rails/config/locales/defaults/en.yml Outdated Show resolved Hide resolved
app-rails/spec/forms/users/new_session_form_spec.rb Outdated Show resolved Hide resolved
SammySteiner and others added 2 commits June 28, 2024 19:01
@SammySteiner
Copy link
Contributor Author

SammySteiner commented Jun 28, 2024

Thank you for working on this!

For app/controllers and app/forms/users, could we use the variable honeypot instead of hp_field, so that the field naming convention is consistent and the purpose of the field is spelled out? Without knowing in advance, I think it is hard to guess what hp stands for in this context.

The guidance is not to have the name or id of the fields include the word honeypot as some bots are programmed to ignore fields with that word in the name or id, which is why I went with hp_field. Here are some alternatives:

  1. pick some other descriptive name like,trap_for_spambots or something else
  2. something like h_o_n_e_y_p_o_t should provide some obfuscation
  3. alias the attribute using alias_attribute in the app/forms/users which while a bit messy will contain or obfuscated name and the word honeypot in both forms and controllers.

i.e. in the app/users/form:

attr_accessor :email, :hp_field 
alias_attribute :honeypot_field, :hp_field
validates :honeypot_field, absence: true

in the controller:

honeypot_field = params[:users_forgot_password_form][:hp_field]
@form = Users::ForgotPasswordForm.new(email: email, honeypot_field: honeypot_field)

params.require(:users_reset_password_form).permit(:email, :code, :password, :hp_field)

Thoughts?

@rocketnova
Copy link
Contributor

The guidance is not to have the name or id of the fields include the word honeypot as some bots are programmed to ignore fields with that word in the name or id, which is why I went with hp_field.

Thank you for explaining. What about spamtrap or spam_trap? I also think hp would be acceptable if we added comments everywhere it appeared to explain what it is.

@SammySteiner
Copy link
Contributor Author

SammySteiner commented Jun 28, 2024

The guidance is not to have the name or id of the fields include the word honeypot as some bots are programmed to ignore fields with that word in the name or id, which is why I went with hp_field.

Thank you for explaining. What about spamtrap or spam_trap? I also think hp would be acceptable if we added comments everywhere it appeared to explain what it is.

Sounds good, I went with spam_trap

Let me know if you think we should also rename the honeypot helper in the form builder to spam_trap? But I like that the honeypot method creates a spam trap 🤷

@SammySteiner SammySteiner requested a review from rocketnova June 28, 2024 23:42
@rocketnova
Copy link
Contributor

Sounds good, I went with spam_trap

👍

Let me know if you think we should also rename the honeypot helper in the form builder to spam_trap? But I like that the honeypot method creates a spam trap 🤷

🤔 I don't feel strongly about it. I think that if it's named the same thing consistently, there's less mental translation that someone has to do as they navigate the code, but I also think we can always rename it in the future if we hear feedback that it is cumbersome.

Copy link
Contributor

@rocketnova rocketnova left a comment

Choose a reason for hiding this comment

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

Thank you!

@SammySteiner SammySteiner merged commit c7e83fa into main Jun 28, 2024
5 checks passed
@SammySteiner SammySteiner deleted the sammysteiner/35-add-honeypot-to-forms branch June 28, 2024 23:54
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.

Honeypot field and validation for forms
2 participants