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
Blocked domains flood gserver entries #12700
Blocked domains flood gserver entries #12700
Conversation
- some trolls managed to flood gserver with useless URLs. They can be blocked by domain blocking them, but still it floods gserver table with dead entries - this hack tries to change that so they won't enter gserver at all. Let's hope these trolls as `activitypub-trolls.cf` learn a lesson (and get adults soon) Signed-off-by: Roland Häder <roland@mxchange.org>
- just for consistency ...
} elseif (Network::isUrlBlocked($url)) { | ||
Logger::info('Server domain is blocked', ['url' => $url]); | ||
return false; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
elseif
here is superfluous since there's a return
in the parent condition.
} elseif (Network::isUrlBlocked($url)) { | |
Logger::info('Server domain is blocked', ['url' => $url]); | |
return false; | |
} | |
if (Network::isUrlBlocked($url)) { | |
Logger::info('Server domain is blocked', ['url' => $url]); | |
return false; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What do you mean by "parent condition"?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The condition where the elseif
originated.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I usually only make a 2nd if()
block if there is code between the upper and the then added lower one:
if (someCondition) {
// Do something, like logging, closing/freeing resources
return false;
}
// Do other things:
doOtherCode();
if (otherCondition) {
// Do something, like logging, closing/freeing resources
return false;
}
Otherwise, when there is no code in between these two if()
blocks, I merge them with elseif()
or when they do the exact same thing, I merge them with ||
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let me provide a more actionable suggestion.
} elseif (Network::isUrlBlocked($url)) { | |
Logger::info('Server domain is blocked', ['url' => $url]); | |
return false; | |
if (!Strings::compareLink($url, $original_url)) { | |
self::setFailureByUrl($original_url); | |
if (!self::getID($url, true)) { | |
self::detect($url, $network, $only_nodeinfo); | |
} | |
return false; | |
} | |
if (Network::isUrlBlocked($url)) { | |
Logger::info('Server domain is blocked', ['url' => $url]); | |
return false; | |
} |
These condition have nothing to do with each other because of the return false
. If there was anything but a return a elseif
would be warranted, but not in this case where it adds complexity for no benefit.
So far really effective! Without this change, both the later one would have flooded again with even millions(!) of sub domains. |
- moved if() block to suggested position by MrPetovan, for me I want to have all conditions checked at the start of the method, e.g. no unwanted null references or (in this case) if the URL is blacklisted - normalized URLs are without SSL, means http://host/path/file.ext so they exist only once for contacts and servers (aka. instances) - documented returned type `void`
Logger::info('Set failed status for existing server', ['url' => $url]); | ||
if (self::isDefunct($gserver)) { | ||
self::archiveContacts($gserver['id']); | ||
} | ||
return; | ||
} | ||
self::insert(['url' => $url, 'nurl' => Strings::normaliseLink($url), | ||
|
||
if (Network::isUrlBlocked($url)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This block here is not needed. AFAIK we don't call this function at all for blocked servers. If we should, then we had to check earlier for blocked servers and should call the appropriate function from there.
This is killing my power bill, I have 1000 of duplicate entry errors, because of this same issue. |
Can you please share a sample of duplicate entry errors? |
Ok, I forgot to respond to this sooner, Event details
Datacode2006 errorMySQL server has gone away callstackGServer::insert, GServer::setFailure, GServer::detect, GServer::check, GServer::getID, GServer::add, UpdateServerPeers::execute, Worker::execFunction paramsINSERT INTO `gserver` (`url`, `nurl`, `network`, `created`, `last_failure`, `failed`) VALUES ('https://32tbqwum0.activitypub-troll.cf', 'http://32tbqwum0.activitypub-troll.cf', 'unkn', '2023-04-12 12:50:56', '2023-04-12 12:50:56', 1)
Datacode1062 errorDuplicate entry 'http://39cqpqb9m.activitypub-troll.cf' for key 'nurl' callstackGServer::insert, GServer::setFailure, GServer::detect, GServer::check, GServer::getID, GServer::add, UpdateServerPeers::execute, Worker::execFunction paramsINSERT INTO `gserver` (`url`, `nurl`, `network`, `created`, `last_failure`, `failed`) VALUES ('https://39cqpqb9m.activitypub-troll.cf', 'http://39cqpqb9m.activitypub-troll.cf', 'unkn', '2023-04-09 15:03:00', '2023-04-09 15:03:00', 1) |
Can you please confirm what version of Friendica you are running and what pattern you set in your server block list related to this attack? |
https://www.urbanmind.net/friendica Using PiHole blacklisting, with (.|^)activitypub-troll.cf$ and cloudflare dns firewall. |
Ok, the |
So, I have looked into the chain of calls you provided, and it should have been stopped in 4 different places given the error and your blocked server domain pattern:
I don't have a good explanation for this. I also note that we've added a number of checks against the server domain pattern block list in the upcoming version 2023.04 so updating to it should reduce your woes. |
The CSS not loading, was not intended, but it helps. I think I fixed the css not loading issue, it was hidden in my web shield settings. |
It has shown in the past that trolls from the infamous
gab.best
and also more recentlyactivitypub-troll.cf
domains have used a flaw in Friendica to flood thegserver
table with millions (see this link) of useless records (tons of invalid and not findable sub domains) of records that just pile up and seem to never end.With this change, I was finally able to block them from entering my and other's database tables. This PR also contains a change to to avoid redundant invocations of
Strings::normaliseLink()
which can safe some CPU cycles and speedup the code.