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

[RFC] Hide captcha field for users and add honeypot field #832

Merged
merged 4 commits into from May 19, 2017

Conversation

ausi
Copy link
Member

@ausi ausi commented May 18, 2017

With these changes we would achieve the following:

  1. Normal users won’t see the captcha field and can just submit the form.
  2. If a user is detected as a spam bot, they can still send the form by answering the captcha question.
  3. Bots that don’t support JS, or that can’t resist the honeypot cannot send the form.

@leofeyer leofeyer added this to the 4.4.0 milestone May 19, 2017
@leofeyer leofeyer self-assigned this May 19, 2017
@leofeyer leofeyer merged commit a8695f0 into contao:develop May 19, 2017
@leofeyer
Copy link
Member

Awesome, thank you @ausi.


<?php if (!$this->hasErrors()): ?>
<div style="display:none">
<label for="ctrl_<?= $this->id ?>_hp">Do not fill out this field</label>
Copy link
Member

Choose a reason for hiding this comment

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

In English should be fill in this field and I wonder if we should translate it anyway for screenreaders? Also I wonder if there's some WAI-ARIA directive to improve usability for handicapped people.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, the PR wasn’t completely finished, thats why I added [RFC], not [RTM] :)
Because there is a display:none div directly around it, no user and no screenreader user will see that in 99.9999% of cases.
But yes, we should change it to Do not fill in this field and translate it.

Copy link
Member

Choose a reason for hiding this comment

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

With display:none, the message will never be read out by screen readers. We do not need to translate it therefore.

Copy link
Member

Choose a reason for hiding this comment

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

Fixed in 820ea42.

@ausi
Copy link
Member Author

ausi commented May 19, 2017

@leofeyer should we restructure getQuestion() and getSum() so that the it works independend of the order and how often you call these methods? Calling getQuestion() twice, would overwrite the session, and calling getSum() before getQuestion() doesn’t work.

@ausi
Copy link
Member Author

ausi commented May 19, 2017

Followup: #835

@fenepedia
Copy link

@ausi Does this new function replace your anti-spam-extension? In other words, is it the same?

@ausi
Copy link
Member Author

ausi commented May 20, 2017

It works with the same principles: time check, JS check and a honeypot field. So I think it should offer the same protection.

The biggest difference to the anti-spam extension is the naming of the honeypot field I think. The extension tries to use very attractive names, but this can have conflict problems with other fields in the form.

@leofeyer
Copy link
Member

We could improve the new feature like "if there is no field named email, name the CAPTCHA field email; if there is no field named name, name the CAPTCHA field name; and use the CAPTCHA field name with a suffix in all other cases". (Then the form would have to set the CAPTCHA field name I guess, because the widget does not know about the other widgets.)

Does this make sense? And is it worth the effort?

@Toflar
Copy link
Member

Toflar commented May 22, 2017

The "if there is no email" does not work reliably. You could determine that in the loadFormField hook in case of the form generator but the captcha is used elsewhere too. Like comments form or whatever the developers did directly with that widget. I think the honeypot field is going to work anyway.

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

Successfully merging this pull request may close these issues.

None yet

4 participants