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

Add a honeypot field to collections.add form to catch spammy bots (bug 1211012). #828

Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
19 changes: 18 additions & 1 deletion apps/bandwagon/forms.py
Expand Up @@ -6,6 +6,7 @@

import commonware.log
from tower import ugettext as _, ugettext_lazy as _lazy
from django_statsd.clients import statsd

import amo
from amo.utils import clean_nl, has_links, slug_validator, slugify
Expand Down Expand Up @@ -48,7 +49,6 @@ class AddonsForm(Form):
required=False)

def clean_addon(self):

addons = []
for a in self.data.getlist('addon'):
try:
Expand Down Expand Up @@ -136,13 +136,30 @@ class CollectionForm(ModelForm):
icon = forms.FileField(label=_lazy(u'Icon'),
required=False)

# This is just a honeypot field for bots to get caught
# L10n: bots is short for robots
your_name = forms.CharField(
label=_lazy(
u"Please don't fill out this field, it's used to catch bots"),
Copy link
Contributor

Choose a reason for hiding this comment

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

If you add a comment above this formatted like # L10n: bots is short for robots then the localizers will see it in their web interface. It might help them out since English colloquialism can trip them up.

required=False)

def __init__(self, *args, **kw):
super(CollectionForm, self).__init__(*args, **kw)
# You can't edit the slugs for the special types.
if (self.instance and
self.instance.type in amo.COLLECTION_SPECIAL_SLUGS):
del self.fields['slug']

def clean(self):
Copy link
Contributor

Choose a reason for hiding this comment

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

could instead be done in clean_your_name right? If you put it in clean on purpose (to not have the error message associated with this field maybe? So it's not obvious that it's a honeypot?) then please add a comment here explaining that.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, I didn't wanted it related to a field in the error messages so that it could be shown eventually at the top of the form in the __all__ section.

Copy link
Contributor

Choose a reason for hiding this comment

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

Makes sense. So please add a comment explaining that ;) thanks!

# Check the honeypot here instead of 'clean_your_name' so the
# error message appears at the top of the form in the __all__ section
if self.cleaned_data['your_name']:
statsd.incr('collections.honeypotted')
Copy link
Contributor

Choose a reason for hiding this comment

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

in addition to this, I'd suggest also adding a log statement because grepping the logs could be helpful

log.info('Bot trapped in honeypot at collections.create')
raise forms.ValidationError(
"You've been flagged as spam, sorry about that.")
return super(CollectionForm, self).clean()

def clean_name(self):
name = self.cleaned_data['name']
if BlacklistedName.blocked(name):
Expand Down
3 changes: 3 additions & 0 deletions apps/bandwagon/templates/bandwagon/includes/addedit.html
@@ -1,6 +1,9 @@
{{ csrf() }}

<fieldset>
{# Honeypot field for bots to get caught #}
{{ form.your_name }}

{{ field(form.name, _('Name:'), class='long') }}
{% if form.slug %}
<p id="collection-form-slug">
Expand Down
53 changes: 53 additions & 0 deletions apps/bandwagon/tests/test_views.py
Expand Up @@ -386,6 +386,11 @@ def create_collection(self, **kw):
eq_(r.status_code, 200)
return r

@patch('bandwagon.views.statsd.incr')
def test_create_collection_statsd(self, mock_incr):
self.client.post(self.add_url, self.data, follow=True)
mock_incr.assert_any_call('collections.created')

def test_restricted(self, **kw):
g, created = Group.objects.get_or_create(rules='Restricted:UGC')
self.client.login(username='clouserw@gmail.com',
Expand Down Expand Up @@ -1272,3 +1277,51 @@ def test_clean_description(self):
'description': '<a href="http://example.com">example.com</a>'}
with self.assertRaisesRegexp(ValidationError, 'No links are allowed'):
form.clean_description()

def test_honeypot_not_required(self):
author = UserProfile.objects.get(pk=9945)

form = forms.CollectionForm(
initial={'author': author},
data={
'name': 'test collection',
'slug': 'test-collection',
'listed': False,
}
)

assert form.is_valid()

def test_honeypot_fails_on_entry(self):
author = UserProfile.objects.get(pk=9945)

form = forms.CollectionForm(
initial={'author': author},
data={
'name': 'test collection',
'slug': 'test-collection',
'listed': False,
'your_name': "I'm a super dumb bot",
}
)

assert not form.is_valid()
assert 'spam' in form.errors['__all__'][0]

@patch('bandwagon.forms.statsd.incr')
def test_honeypot_statsd_incr(self, mock_incr):
author = UserProfile.objects.get(pk=9945)

form = forms.CollectionForm(
initial={'author': author},
data={
'name': 'test collection',
'slug': 'test-collection',
'listed': False,
'your_name': "I'm a super dumb bot",
}
)

assert not form.is_valid()

mock_incr.assert_any_call('collections.honeypotted')
2 changes: 2 additions & 0 deletions apps/bandwagon/views.py
Expand Up @@ -13,6 +13,7 @@
import caching.base as caching
import commonware.log
from tower import ugettext_lazy as _lazy, ugettext as _
from django_statsd.clients import statsd

import amo
from amo import messages
Expand Down Expand Up @@ -330,6 +331,7 @@ def add(request):
if aform.is_valid():
aform.save(collection)
collection_message(request, collection, 'add')
statsd.incr('collections.created')
log.info('Created collection %s' % collection.id)
return http.HttpResponseRedirect(collection.get_url_path())
else:
Expand Down
5 changes: 5 additions & 0 deletions static/css/zamboni/zamboni.css
Expand Up @@ -3396,6 +3396,11 @@ td.input {
padding: 1em;
}

/* Hide this honeypot field for our users */
.collection-create input[name="your_name"] {
display: none;
Copy link
Contributor

Choose a reason for hiding this comment

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

I believe some (most?) of the bots out there know how to detect a display: none css property. But we don't want to over-engineer this thing unless we have to. So maybe add some metrics so we at least know if we're catching some of them, and then we'll be able to refine if needed?

}

.contributor span {
position: relative;
}
Expand Down