fix(donation): prevent WP users from being created if donation fails

[mathetos]

User Story

As a Give Admin, I am experiencing a lot of spam user registrations through my Give donation forms. I want to prevent that from happening.

Current Behavior

I currently am using both the Akismet integration and also the functionality plugin Stop Donor Spam, but these spam users are still being created.

Expected Behavior

I expect all wordpress users generated through our Give forms to be actual donors, and for spam donors to be blocked.

Possible Solution

Check on the order of actions during donation. @kevinwhoffman and I have a hunch that the user generation action is happening BEFORE the donation has been validated. Ideally, user creation should only happen after the donation itself has been completely successful and passed all validation checks.

Steps to Reproduce

1. We have a local site that has a lot of spam users that were created via Give forms.

Related

Customer Reports

• https://secure.helpscout.net/conversation/556699222/16607/?folderId=672197
• Search for all HS tickets with the tag donor spam https://secure.helpscout.net/search/?query=tag:donor%20spam

• Related Github Issues

• fix(donation): prevent $0 donation spam #2930
• feat(form): disallow email address as first name #2862
• Donation form should have nonce verification. #2568

Tasks

• Reorder the actions (if that is the cause of this issue of course)
• Do test donations that should intentionally get rejected by Akismet and verify whether the WP user was created while the donation was rejected.
• Test the Related Github issues in combination with this fix to ensure all new spam prevention methods are still working as intended.

[mehul0810]

After discussion with @ravinderk over the call, we have finalized 2 tasks for now.

@kevinwhoffman Can you provide me the local site you're talking about it over developer call so that I can do research on that data and try to identify whether there is any other scenario to handle for preventing spam.

[DevinWalker]

@mehul0810 here's the export that you can import into local: https://givewp.slack.com/files/U064V9KDJ/FA94LKFB6/20180412_alphacare_41d61024534092be2914180412174723_archive.zip

[kevinwhoffman]

@mehul0810 The first step I would take is to dig into the process by which users are registered in the donation form and determine whether a user is created before the donation is processed. If that is the case, then we will want to re-order the user creation process so that it occurs only if the donation post has been created.

Please investigate this on a fresh install of Give and report back. Then we can decide how to proceed with the backup site if necessary.

[mehul0810]

@kevinwhoffman @ravinderk @DevinWalker
I've done a research on how to prevent the donation/donor/user spam occurred with Give.

Findings

1. We are allowing Alphanumeric in First Name and Last Name ( this is the pattern I've noticed with the site dump provided which we can even solve by fixing the point (2) )
2. When donation form supports user registration, the user is created before the donation is validated ( checked with fresh as well as an existing instance )
3. We're returning gateway even if the form has hidden gateway variable not passed with it. ( this is the case in the spamming script which I've created to test )

Regarding Point (2): We need to validate donation and then create a user so the sequence of the hook being called will also change. Let me know your thoughts whether we do require the backward compatibility in this case?

Also, I'll try to optimize the code to make sure that we bail out earlier if any of the required data is not passed. This will also help to prevent donation/donor/user spam.

[kevinwhoffman]

When donation form supports user registration, the user is created before the donation is validated ( checked with fresh as well as an existing instance )

This seems to be the core of the problem and explains why some customers are seeing spam users but not spam donations. We should only register the user after the donation is validated.

We need to validate donation and then create a user so the sequence of the hook being called will also change. Let me know your thoughts whether we do require the backward compatibility in this case?

@mehul0810 I'm not seeing the backwards compatibility concerns that would arise by only registering users upon valid donations. Can you elaborate?

[mehul0810]

I'm not seeing the backwards compatibility concerns that would arise by only registering users upon valid donations. Can you elaborate?

@kevinwhoffman I had a doubt regarding backward compatibility in this case. Hence, I've taken this point into consideration to ensure that backward compatibility is required or not based on individual thoughts.

[kevinwhoffman]

I see. I think it depends on whether the data passed in those hooks changes as a result of the sequence change. @ravinderk and @DevinWalker will have better insight.

[ravinderk]

Slack Chat Summary

Participants: @mehul0810, @Ravinder
Topic: discuss backward compatibility issue within donation workflow if we will change user registration point

Result: @mehul0810 will research on it and give use suspicious code examples

[mehul0810]

Slack Call Summary

Participants: @mehul0810 @ravinderk
Topic: Discuss backward compatibility by moving user created after the donation is created
Result: I'll research on validating donation fields before creating user will work or not.

[mehul0810]

Slack Call Summary

Participants: @mehul0810 @ravinderk
Topic: Discuss backward compatibility
Result: Discussed whether the donor is created or user is created earlier and will test some sample donations to ensure that user/donor is created earlier. Also, will check whether the user is created with Give Donor role from other plugins or not.

[mehul0810]

Slack Call Summary

Participants: @mehul0810 @ravinderk
Topic: Discuss backward compatibility
Result: It seems that we can implement some of the minor changes but we need to discuss moving user after the donation is validated because we noticed that changing the flow of user > donation > donor flow can affect the existing users. Hence, we need to discuss this over the call with @DevinWalker and @kevinwhoffman

[kevinwhoffman]

We're approaching 2.1 release and still need to discuss the implications of reordering the user registration flow per @mehul0810's comment #3035 (comment).

Let's return to this immediately after 2.1 release.

[mehul0810]

@DevinWalker @kevinwhoffman @ravinderk

Final Suggestions for spam prevention implementation

1. Client and Server side validation to allow only alphabets for first and last name
2. Rather than re-ordering the user creation process, for now, I would suggest adding a hidden email field ( i.e. with name give_email_2) which will be empty when a donor actually donates. But, when a spam bot tries to donate via donation form, then it will get filled in. So, there are 2 options we can do.
(a) Add a conditional check to bail out if we have non-empty email field give_email_2, considering it as a spam.
(b) Flag the donation as spam, if we have non-empty email field give_email_2 so that if the donation incorrectly gets registered as spam then admin can manually set as not spam.
3. If we create a donation spam script and keep the gateway field empty then also donation processed successfully. So, to solve this, don't process donation if gateway field is not found.
4. Sanitize all the input fields as there are lots of fields not sanitized in the process of donation.

Let me know your thoughts on proceeding for implementation of spam prevention and see whether users still get spam donation or not. After releasing this fix, if end users still get spam donations/donors/users then we will look forward to re-ordering the user creation process. Also, let me know with which option (a) or (b) to proceed with for point (2).

[ravinderk]

@mehulgohil how you suggestion different then honeypot hidden field
which we already have in core

for ref: https://github.com/WordImpress/Give/blob/master/includes/forms/template.php#L147

[DevinWalker]

@ravinderk that was my thought too - we already have a honeypot field, and it apparently doesn't work very well.

[slewisma]

Two thoughts from my own experience and seeing what some other form tools do to fight spam:

For the honeypot input that is already built-in, don't use "honeypot" in its id, name or css class. Bot authors are smart enough to look for that sort of thing and skip the input field. @mehul0810's idea differs from the current implementation in this sense. His suggested approach makes the field more tempting to the bots' scripts.

Consider building in optional support for noCaptcha or reCaptcha for users willing to go get an API key to enable them.

[mathetos]

I like options 1, 3, and 4.

✔️ Option 1 -- I think is easy and obvious and will be a minor benefit.

✔️ Option 3 -- I like this in general, and it could have a nice clean effect on our forms. basically, if NO gateway is set by default then none of those fields are visible until one is selected. That has a nice progressive effect. But it does require the donor to do an additional click (for those that would have previously just donated with the default gateway)

✔️ Option 4 -- This is also important and obvious. I think this and option 1 alone will make a big difference.

❌ Option 2 feels like it could raise more problems than it solves. I think that bots are smart enough to avoid inputs that are marked as "honeypot" (like Scott suggested) but also smart enough to avoid inputs that are hidden as well.

❌ I don't support offering noCaptcha or reCaptcha in our donation forms as an option at all for a wide variety of reasons, most importantly it's just a bad donor experience.

[kevinwhoffman]

I agree with @mathetos on all of the above.

In addition, only registering users after a donation is successfully created ensures that other anti-spam tactics like Akismet and Stop Donor Spam will be more successful. Specifically, we know that in the past Stop Donor Spam has prevented spam donations but allowed spam users. So by making the donation a precondition for user registration, that situation should be improved.

[mathetos]

That's right, I also STRONGLY agree that the actions of how things are triggered matters a lot. In my mind it should be something like this:

• Validate all fields
  |--> If all fields valid, create donation meta
  |------> If donation meta is created, send the donation to the payment gateway
  |----------> If the payment gateway returns a confirmation, create the user
  |--------------> Send to the donation confirmation page, and show a message about their created user account (maybe an error message if user creation failed).

[DevinWalker]

@mehul0810 it sounds like we're in agreement in the above comments. I'm removing this from the blockers so we can proceed.

[mehul0810]

Slack Call Summary

Participants: @ravinderk @mehul0810
Topic: Discussion on $_REQUEST used and repetitive sanitization of data
Result: As discussed, we will replace usage of $_REQUEST with $_POST to strengthen security and I'll create a separate issue to handle repetitive sanitization of data.

[mehul0810]

Slack Call Summary

Participant: @ravinderk @mehul0810
Topic: Discussion on changing workflow for user creation
Result: There are some challenges I am facing with changing the workflow for user creation. I need to move fn call give_insert_payment before give_gateway_* action hook which is a breaking change as well as will need to refactor all the add-ons. So, I'll add a detail description within the PR I'll create. Also, I'll add an additional nonce check for user creation as well as login on donation form to make sure that any bot scripts don't go through.

[mehul0810]

I have implemented the below tasks to handle the donation spam for now.

• Client and Server side validation to allow only alphabets for first and last name
• If we create a donation spam script and keep the gateway field empty then also donation processed successfully. So, to solve this, don't process donation if gateway field is not found.
• Sanitize all the input fields as there are lots of fields not sanitized in the process of donation.

In future, we will refactor the user creation work-flow based on the donation spam user reports after this issue as it will be a breaking change.

[mathetos]

@mehul0810 Just thinking through possible problems, how does your validation of first/last name handle non-latin characters (Chinese/Japanese, etc)? Or characters with accents?

[raftaar1191]

@mehul0810 @mathetos

I just tested by adding a name in Chinese language and it showing me a validation error

[DevinWalker]

This needs to be fixed before we can release 2.1.3