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

IP address check will produce a lot of false positives #177

Closed
websupporter opened this issue Apr 2, 2018 · 8 comments · Fixed by #194
Closed

IP address check will produce a lot of false positives #177

websupporter opened this issue Apr 2, 2018 · 8 comments · Fixed by #194

Comments

@websupporter
Copy link
Contributor

With the GDPR coming, a lot of people will delete the IP address automatically, leaving an empty string or 127.0.0.1 or something similar. If this happens before our IP comparison, it will produce a lot of false positives. If it happens after our IP comparison, the comparison is useless.

We should consider to refactor this check.

This is, what I see right now:

  1. Get rid of the check completely
  2. Decouple email/IP/website-check, which is currently done in one step and give the user the ability to remove only the IP check

We should do this quite soon.

@stklcode
Copy link
Contributor

stklcode commented Apr 2, 2018

Are there any real world numbers on the percentage of Spam blocked by IP checks?
I just got numbers for login attempts where single IP checks are far less useful than subnet comparisons, as many botnets operate on some kind of round-robin rotation.

Generally, version 2, separating the steps technically, might be a good idea, even if the IP filter is dropped.

If the filter stays, I'd like to put one more suggestion into discussion, that is add a filter to automatilly skip the check, even if enabled, on such "addresses". Could match empty strings and local addresses (loopback and server's IP(s)) by default. This also fixes potential pre-existing issues with intransparent or just badly configured reverse proxies.

@Zodiac1978
Copy link
Member

There are at least two occurrences to check:

  • IP check itself (hostname to IP/IP to hostname check). This is not recommended to use because of load balancers, etc.

  • IP comparison in local spam database. This could be skipped if the IP is empty (maybe already the case) or if the IP is 127.0.0.1.

@krafit krafit modified the milestones: 2.7.2, 2.8.0 Apr 20, 2018
@websupporter
Copy link
Contributor Author

I think the cleanest way forward here would be remove the IP-check for the local database, but leave the others in place.

@websupporter
Copy link
Contributor Author

@Zodiac1978
Copy link
Member

Am I missing something or is the IP-check not removed? @websupporter

@websupporter
Copy link
Contributor Author

@Zodiac1978, after some legal consultation by @krafit, we came to the conclusion, we can save a hash of the IP and check the IP of the currently incoming comment with the hashes, we have saved in the database.

@Zodiac1978
Copy link
Member

@websupporter This is not a legal question. It is about: What happens if every IP is changed to 127:0:0:1 or an empty value. Does it break our checks?

@websupporter
Copy link
Contributor Author

No @Zodiac1978. We save it by our own. What I mean, we have WP plugins out there, which alter the IP address when the comment is saved. So we shouldn't rely on the saved data in wp_comments no longer. Instead, we are building our own database with hashes, which we directly generate while saving the comment.

What we rely on is $_SERVER['REMOTE_ADDR'] to be populated.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants