search

impress-org / givewp

GiveWP - The #1 Donation Plugin for WordPress. Easily accept donations and fundraise using your WordPress website.
https://givewp.com/
GNU General Public License v3.0
340 stars 191 forks source link

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

Closed mathetos closed 6 years ago

mathetos commented 6 years ago

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

Tasks

mehul0810 commented 6 years ago

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 commented 6 years ago

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

kevinwhoffman commented 6 years ago

@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 commented 6 years ago

@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 commented 6 years ago

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 commented 6 years ago

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 commented 6 years ago

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 commented 6 years ago

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 commented 6 years ago

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 commented 6 years ago

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 commented 6 years ago

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 commented 6 years ago

We're approaching 2.1 release and still need to discuss the implications of reordering the user registration flow per @mehul0810's comment https://github.com/WordImpress/Give/issues/3035#issuecomment-383888560.

Let's return to this immediately after 2.1 release.

mehul0810 commented 6 years ago

@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 commented 6 years ago

@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 commented 6 years ago

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

slewisma commented 6 years ago

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 commented 6 years ago

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 commented 6 years ago

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 commented 6 years ago

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:

DevinWalker commented 6 years ago

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

mehul0810 commented 6 years ago

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 commented 6 years ago

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 commented 6 years ago

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

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 commented 6 years ago

@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 commented 6 years ago

@mehul0810 @mathetos

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

image

DevinWalker commented 6 years ago

This needs to be fixed before we can release 2.1.3