Hi,

If we put '0' (Zero) as value in honeypot element, honeypot is not able to restrict the form for submit.
What I see in code, honeypot element check as !empty to show a error and 0 not satisfied the condition hence not detecting the value and pass the same for submit.

Thanks in advance

CommentFileSizeAuthor
#18 2892580-18-honeypot-strict-emptiness.patch1.74 KBgeerlingguy
#17 2892580-17-honeypot-strict-emptiness.patch1.17 KBgeerlingguy
#17 2892580-17-honeypot-strict-emptiness-test-only.patch1.17 KBgeerlingguy
#13 2892580-12-honeypot-strict-emptiness.patch1.63 KBmr.baileys
#13 2892580-12-honeypot-strict-emptiness-test-only.patch1.08 KBmr.baileys
#9 honeypot-restrict_0_value-2892580-1.patch557 bytesonly4kaustav
#7 honeypot-restrict_0_value-2892580-0.patch555 bytesonly4kaustav
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

only4kaustav created an issue. See original summary.

geerlingguy’s picture

geerlingguy CreditAttribution: geerlingguy at Midwestern Mac, LLC commented
Category: Bug report » Support request
Priority: Major » Normal
Status: Active » Postponed (maintainer needs more info)

Can you give a little more detail, like a set of steps I could follow to reproduce this issue?

Are you adding 0 as the 'Honeypot element name' on the configuration page? Or for the time limit? Or is it something you're doing in a custom API integration?

geerlingguy’s picture

geerlingguy CreditAttribution: geerlingguy at Midwestern Mac, LLC commented

Or maybe do you mean that on the site's frontend, if you visit a Honeypot-protected form, and enter 0 as the value in the invisible honeypot field, then Honeypot allows the form to be submitted, even though there technically was a value in the field?

only4kaustav’s picture

only4kaustav CreditAttribution: only4kaustav commented

Hi @geerlingguy,

Your assumption of #3 is right.

Thanks

geerlingguy’s picture

geerlingguy CreditAttribution: geerlingguy at Midwestern Mac, LLC commented
Version: 7.x-1.22 » 7.x-1.x-dev
Category: Support request » Bug report
Status: Postponed (maintainer needs more info) » Active

Sounds like a bug! Strict typing would catch that, but in the meantime, this should be fixed and a test case added to make sure it doesn’t happen again. Switching to 'bug report'.

only4kaustav’s picture

only4kaustav CreditAttribution: only4kaustav commented
Assigned: Unassigned » only4kaustav
only4kaustav’s picture

only4kaustav CreditAttribution: only4kaustav as a volunteer commented
Status: Active » Needs review
FileSize
honeypot-restrict_0_value-2892580-0.patch555 bytes

This patch restrict 0 value of honeypot field element, which was allowed before but should not be.

Status: Needs review » Needs work

The last submitted patch, 7: honeypot-restrict_0_value-2892580-0.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

only4kaustav’s picture

only4kaustav CreditAttribution: only4kaustav as a volunteer commented
Status: Needs work » Needs review
FileSize
honeypot-restrict_0_value-2892580-1.patch557 bytes

Rectify the 5 test case fauled issue.

geerlingguy’s picture

geerlingguy CreditAttribution: geerlingguy at Midwestern Mac, LLC commented
Status: Needs review » Needs work
Issue tags: -bug

Looks good, just needs a test. Then we'll need to port this test and fix to Drupal 8 as well.

only4kaustav’s picture

only4kaustav CreditAttribution: only4kaustav as a volunteer commented
Status: Needs work » Needs review
Chris Matthews’s picture

Chris Matthews CreditAttribution: Chris Matthews commented
Status: Needs review » Needs work
Issue tags: +Needs tests, +needs port to Drupal 8
Parent issue: » #3095205: Plan for Honeypot 7.x-1.26 release

@only4kaustav, the status of this issue should be 'Needs work' as it still needs a test.

mr.baileys’s picture

mr.baileys
Primary language Dutch
Location 🇧🇪 (Ghent)
CreditAttribution: mr.baileys at SQLI - Belgium commented
Assigned: only4kaustav » Unassigned
Status: Needs work » Needs review
Issue tags: -Needs tests
FileSize
2892580-12-honeypot-strict-emptiness-test-only.patch1.08 KB
2892580-12-honeypot-strict-emptiness.patch1.63 KB

Test added.

geerlingguy’s picture

geerlingguy CreditAttribution: geerlingguy at Midwestern Mac, LLC commented
Status: Needs review » Reviewed & tested by the community

LGTM, going to merge this patch to D7, and once that's done change version on this to D8 for the port.

geerlingguy’s picture

geerlingguy CreditAttribution: geerlingguy at Midwestern Mac, LLC commented
Version: 7.x-1.x-dev » 8.x-1.x-dev
Status: Reviewed & tested by the community » Patch (to be ported)

Patch is in 7.x-dev, needs to be ported now to D8.

geerlingguy’s picture

geerlingguy CreditAttribution: geerlingguy at Midwestern Mac, LLC commented
Status: Patch (to be ported) » Needs review
FileSize
2892580-17-honeypot-strict-emptiness-test-only.patch1.17 KB
2892580-17-honeypot-strict-emptiness.patch1.17 KB

Attaching ported patches; worked locally, just want to see what testbot thinks.

geerlingguy’s picture

geerlingguy CreditAttribution: geerlingguy at Midwestern Mac, LLC commented
FileSize
2892580-18-honeypot-strict-emptiness.patch1.74 KB

Eh... uploaded the same patch twice. I hate patch workflows :P — correct full patch attached to _this_ comment.

  • geerlingguy committed 295921f on 8.x-1.x 
    Issue #2892580 by geerlingguy, only4kaustav, mr.baileys: Form is not...
geerlingguy’s picture

geerlingguy CreditAttribution: geerlingguy at Midwestern Mac, LLC commented
Status: Needs review » Fixed

Fixed in 7.x and 8.x.

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.

ciss’s picture

ciss CreditAttribution: ciss at yousign GmbH commented

I feel like this solution is needlessly convoluted. In my opinion this check should suffice:

if ($element['#value'] !== '') {