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

EnTeQuAk · 2015-10-20T20:42:48Z

No description provided.

tofumatt · 2015-10-20T20:46:03Z

apps/bandwagon/forms.py

+    def clean(self):
+        if self.cleaned_data['your_name']:
+            raise forms.ValidationError(
+                'You\'ve been flagged as spam, sorry about that.')


I used to think we had thoughts on strings re: \ in our style guide but I checked and we don't 😒

I always figured it was nicer to use double quotes for strings that would need escaping (eg "It's" over 'It\'s'), but if other olympia code uses \ then stick with this.

This string should get passed through _ like in line 157.

Because this string is never intended to be read by an end user and never shown in the UI, but only to trip up bots, I said that localisation is not needed.

tofumatt · 2015-10-20T20:47:21Z

r+wc (Feel free to ignore my nitpick if using single-quotes is consistent with olympia--consistency is best!)

mstriemer · 2015-10-20T20:52:44Z

Commit message is a bit too long.

EnTeQuAk · 2015-10-20T21:07:26Z

I updated the quotes, found other places where it's being used like that.

magopian · 2015-10-21T08:35:18Z

apps/bandwagon/forms.py

@@ -143,6 +145,12 @@ def __init__(self, *args, **kw):
                self.instance.type in amo.COLLECTION_SPECIAL_SLUGS):
            del self.fields['slug']

+    def clean(self):


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.

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.

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

magopian · 2015-10-21T08:39:21Z

This is nice and concise, I like it (and the tests are great). A small nitpick, then if you could squash your commits (yeah, I know), and we'll be able to merge ;)

Thanks!

r+wc

muffinresearch · 2015-10-21T11:29:27Z

apps/bandwagon/forms.py

@@ -136,6 +135,9 @@ class CollectionForm(ModelForm):
    icon = forms.FileField(label=_lazy(u'Icon'),
                           required=False)

+    # This is just a honeypot field for bots to get caught
+    your_name = forms.CharField(widget=forms.HiddenInput(), required=False)


I don't think if this is just type="hidden" it will work as a honeypot field. It's likely to get ignored by a bot. Typically a honeypot field should be a normal field hidden with CSS. It should also be labelled in such a way to make sure people with assistive tech know to not fill it in.

In my experience it doesn't make any big difference for all the very dumb bots but assistive tech is a good point. display:none; should cover this case completely too, does it not?

I think we'd still want a label just to allow for the absolute worst case that CSS wasn't applied.

muffinresearch · 2015-10-21T11:31:23Z

r- on the basis I'm not sure a hidden field will be likely to be effective on its own.

muffinresearch · 2015-10-21T11:34:00Z

apps/bandwagon/forms.py

@@ -143,6 +145,12 @@ def __init__(self, *args, **kw):
                self.instance.type in amo.COLLECTION_SPECIAL_SLUGS):
            del self.fields['slug']

+    def clean(self):
+        if self.cleaned_data['your_name']:
+            raise forms.ValidationError(


This might not want to raise a Validation error. It should probably look like a success and be quietly binned.

I'd actually even raise a BadRequest or something in case it's filled out - a validation error was just the simplest way to go and avoid special-case handling in the view.

How'd you simply ignore this quietly? Just don't do anything and reload the form (as in clear all data and show an empty form again?)

The point is if you show an error you're making it obvious that the honeypot field was tripped. But if we're assuming that bots are super dumb then that won't matter necessarily. We should definitely instrument this so we know if it's catching anything at all.

muffinresearch · 2015-10-21T11:44:10Z

Another consideration is to look at existing packages adding bolt-on honeypot field functionality. E.g. django-honeypot etc. (Not that I'm advocating any particular package).

EnTeQuAk · 2015-10-21T16:41:33Z

@muffinresearch existing packages do really weird stuff sometimes - specifically django-honeypot is imho way to overkill for a simple honeypot field.

muffinresearch · 2015-10-21T17:26:10Z

@muffinresearch existing packages do really weird stuff sometimes - specifically django-honeypot is imho way to overkill for a simple honeypot field.

Maybe, it was merely a suggestion to consider prior art, in-case someone else has a solution that's been battle tested and already gone through iterations to improve it.

EnTeQuAk · 2015-10-21T21:25:15Z

Okay, unless I forgot something I made all changes discussed above. Ready for re-review :)

magopian · 2015-10-22T08:47:46Z

apps/bandwagon/forms.py

@@ -143,6 +148,15 @@ def __init__(self, *args, **kw):
                self.instance.type in amo.COLLECTION_SPECIAL_SLUGS):
            del self.fields['slug']

+    def clean(self):
+        # Raise the honeypot validation error here to
+        # keep it being rased in the __all__ section of the


typo raised.
Not sure the sentence makes much sense (at least to me), maybe Check the honeypot here instead of 'clean_your_name' so the error message appears at the top of the form in the __all__ section?

magopian · 2015-10-22T08:51:56Z

I think we're good to merge, if you could please first add some (graphite?) metrics, and then squash your commits?

r+wc

muffinresearch · 2015-10-22T09:58:11Z

I wonder about this being just for this specific case? If we ever need another honeypot field it will require another custom implementation. If we think this is the only place we'll need it then perhaps this is fine rather than blocking on making it reusable.

Fwiw I found the the honeypot field code that was used on the marketplace which is here: https://github.com/cvan/django-potato-captcha - see also https://github.com/mozilla/zamboni/search?p=1&q=potato&utf8=%E2%9C%93 (looks like it might have been extracted out separately).

EnTeQuAk · 2015-10-22T14:32:32Z

@muffinresearch yeah, I'll add statsd and then we'll see how it goes. If there's a need to add this to every form the existing code isn't that much so that it won't hurt to settle on another solution. I'd rather not integrate another dependency just now for these 8 lines of code.

muffinresearch · 2015-10-22T14:38:12Z

Sounds good

EnTeQuAk · 2015-10-22T18:23:04Z

Alright, added statsd integration. Ready for re-review 😃

muffinresearch · 2015-10-23T16:59:19Z

lgtm r+

Thanks for the iterations!

muffinresearch · 2015-10-23T17:22:30Z

Oh and you'll want to squash the commits before you land it as before 👍

kumar303 · 2015-10-23T19:50:15Z

apps/bandwagon/forms.py

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


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.

kumar303 · 2015-10-23T20:05:50Z

r+wc from me.

Also, not mandatory, but as @mstriemer mentioned we tend to follow the git guidelines for commit messages. This helps a little when integrating with other tools.

EnTeQuAk · 2015-10-26T08:34:19Z

Squashed and merged in c348d57, thanks for the review!

tofumatt reviewed Oct 20, 2015
View reviewed changes

EnTeQuAk assigned magopian Oct 20, 2015

magopian reviewed Oct 21, 2015
View reviewed changes

muffinresearch reviewed Oct 21, 2015
View reviewed changes

magopian reviewed Oct 22, 2015
View reviewed changes

Add honeypot field to collections.add (bug 1211012).

48f0713

EnTeQuAk force-pushed the feature/add-honeypot-to-collections-add branch from a1d74eb to 48f0713 Compare October 22, 2015 18:04

EnTeQuAk added 2 commits October 22, 2015 11:05

Update comment regarding honeypot

310f51b

Add statsd integration for collections.created/honeypotted.

30e6e84

kumar303 reviewed Oct 23, 2015
View reviewed changes

Add log and l10n comments.

f0c3488

EnTeQuAk closed this Oct 26, 2015

EnTeQuAk deleted the feature/add-honeypot-to-collections-add branch November 12, 2015 17:43

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

EnTeQuAk commented Oct 20, 2015

tofumatt Oct 20, 2015

mstriemer Oct 20, 2015

andymckay Oct 20, 2015

tofumatt commented Oct 20, 2015

mstriemer commented Oct 20, 2015

EnTeQuAk commented Oct 20, 2015

magopian Oct 21, 2015

EnTeQuAk Oct 21, 2015

magopian Oct 21, 2015

magopian commented Oct 21, 2015

muffinresearch Oct 21, 2015

EnTeQuAk Oct 21, 2015

muffinresearch Oct 21, 2015

muffinresearch commented Oct 21, 2015

muffinresearch Oct 21, 2015

EnTeQuAk Oct 21, 2015

muffinresearch Oct 21, 2015

muffinresearch commented Oct 21, 2015

EnTeQuAk commented Oct 21, 2015

muffinresearch commented Oct 21, 2015

EnTeQuAk commented Oct 21, 2015

magopian Oct 22, 2015

magopian commented Oct 22, 2015

muffinresearch commented Oct 22, 2015

EnTeQuAk commented Oct 22, 2015

muffinresearch commented Oct 22, 2015

EnTeQuAk commented Oct 22, 2015

muffinresearch commented Oct 23, 2015

muffinresearch commented Oct 23, 2015

kumar303 Oct 23, 2015

kumar303 commented Oct 23, 2015

EnTeQuAk commented Oct 26, 2015

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

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

Conversation

EnTeQuAk commented Oct 20, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tofumatt commented Oct 20, 2015

mstriemer commented Oct 20, 2015

EnTeQuAk commented Oct 20, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

magopian commented Oct 21, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

muffinresearch commented Oct 21, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

muffinresearch commented Oct 21, 2015

EnTeQuAk commented Oct 21, 2015

muffinresearch commented Oct 21, 2015

EnTeQuAk commented Oct 21, 2015

Choose a reason for hiding this comment

magopian commented Oct 22, 2015

muffinresearch commented Oct 22, 2015

EnTeQuAk commented Oct 22, 2015

muffinresearch commented Oct 22, 2015

EnTeQuAk commented Oct 22, 2015

muffinresearch commented Oct 23, 2015

muffinresearch commented Oct 23, 2015

Choose a reason for hiding this comment

kumar303 commented Oct 23, 2015

EnTeQuAk commented Oct 26, 2015