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 against Bot registrations #4970

Merged
merged 4 commits into from
May 4, 2024

Conversation

nesges
Copy link
Contributor

@nesges nesges commented Apr 22, 2024

This PR introduces a form-field named "username" to the registration form. This field must not be filled, but it acts as a honeypot for bots. If it is filled the Validator of RegisterController would invalidate a registration attempt. To the human viewer the honeypot is made invisible through css.

The CSS deliberately omits display:none because it can be assumed that bots would easily recognize this. Instead it used a similar CSS like Bootstraps visually-hidden class.

Caveat: To ensure that the form remains accessible, aria-hidden=true is used to tell screenreaders to ignore the honeypot. Bots may be programmed intelligently enough to recognize the attribute and ignore the field. But I do think that accessibility comes first and I haven't seen a bot that recognizes aria-hidden yet

@ssddanbrown
Copy link
Member

Thanks for offering this PR @nesges,
I'm not too keen on playing cat-and-mouse with bots, but since a honeypot like this is quite simple I think this would be a reasonable default addition.

A couple of questions:

  • Is the use of the ambrosia-container class name specific or significant in any way? I understand you need to uniquely target that section but just want to understand that in case of future changes.
  • Would you be able to add a couple of Registration Test cases? Just to ensure the field is on the page, and the request is rejected on that field being filled. (extra guidance on our tests here)

@nesges
Copy link
Contributor Author

nesges commented Apr 23, 2024

The class name ambrosia-container is totally arbitrary and can easily be swapped. I just wanted to avoid naming it honeypot because I'm always overstating bot cleverness.

Will gladly look into the Test cases and add some to RegistrationTest. However, I'm just getting ready to go on vacation and not sure if I can do it at short notice

@ssddanbrown
Copy link
Member

ssddanbrown commented Apr 23, 2024

@nesges thanks for confirming on the class name.

Will gladly look into the Test cases and add some to RegistrationTest. However, I'm just getting ready to go on vacation and not sure if I can do it at short notice

Okay, don't add stress before your vacation then, I can throw these in during testing/review.
Enjoy the vacation!

@nesges
Copy link
Contributor Author

nesges commented Apr 24, 2024

That's kind of you, thanks! :-)

@ssddanbrown ssddanbrown added this to the Next Feature Release milestone May 4, 2024
ssddanbrown added a commit that referenced this pull request May 4, 2024
Also cleaned up old RegistrationController syntax.
Review of #4970
@ssddanbrown ssddanbrown merged commit 0d2a268 into BookStackApp:development May 4, 2024
1 check passed
@ssddanbrown
Copy link
Member

Thanks again @nesges, now merged for next feature release with some testing and minor tweaks made in 5c28bcf.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants