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.

Sign up for GitHub

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

Jump to bottom

Blocked domains flood gserver entries #12700

Merged
merged 3 commits into from
Jan 21, 2023
Merged

Blocked domains flood gserver entries #12700

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
a0704db
Continued:
Quix0r Jan 20, 2023
ba08692
Continued:
Quix0r Jan 20, 2023
27969e8
Continued:
Quix0r Jan 21, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
20 changes: 17 additions & 3 deletions src/Model/GServer.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -458,19 +458,30 @@ public static function setBlockedByUrl(string $url)
*/
public static function setFailureByUrl(string $url)
{
$gserver = DBA::selectFirst('gserver', [], ['nurl' => Strings::normaliseLink($url)]);
$nurl = Strings::normaliseLink($url);

if (Network::isUrlBlocked($url)) {
Logger::info('Server domain is blocked', ['url' => $url]);
return;
} elseif (Network::isUrlBlocked($nurl)) {
Logger::info('Server domain is blocked', ['nurl' => $nurl]);
return;
}
Quix0r marked this conversation as resolved.
Show resolved Hide resolved

$gserver = DBA::selectFirst('gserver', [], ['nurl' => $nurl]);
if (DBA::isResult($gserver)) {
$next_update = self::getNextUpdateDate(false, $gserver['created'], $gserver['last_contact']);
self::update(['url' => $url, 'failed' => true, 'blocked' => Network::isUrlBlocked($url), 'last_failure' => DateTimeFormat::utcNow(),
'next_contact' => $next_update, 'network' => Protocol::PHANTOM, 'detection-method' => null],
['nurl' => Strings::normaliseLink($url)]);
['nurl' => $nurl]);
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),

self::insert(['url' => $url, 'nurl' => $nurl,
'network' => Protocol::PHANTOM, 'created' => DateTimeFormat::utcNow(),
'failed' => true, 'last_failure' => DateTimeFormat::utcNow()]);
Logger::info('Set failed status for new server', ['url' => $url]);
Expand Down Expand Up @@ -560,6 +571,9 @@ private static function detect(string $url, string $network = '', bool $only_nod
self::detect($url, $network, $only_nodeinfo);
}
return false;
} elseif (Network::isUrlBlocked($url)) {
Quix0r marked this conversation as resolved.
Show resolved Hide resolved
Logger::info('Server domain is blocked', ['url' => $url]);
return false;
Comment on lines +575 to +577
Copy link
Collaborator

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.

Suggested change
} 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;

Copy link
Author

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"?

Copy link
Collaborator

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.

Copy link
Author

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 ||.

Copy link
Collaborator

@MrPetovan MrPetovan Jan 21, 2023 •
edited

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.

Suggested change
} 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.

}

$valid_url = Network::isUrlValid($url);
Expand Down
4 changes: 2 additions & 2 deletions src/Worker/UpdateServerPeers.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -57,13 +57,13 @@ public static function execute(string $url)
$total = 0;
$added = 0;
foreach ($peers as $peer) {
if (Network::isUrlBlocked('http://' . $peer)) {
if (Network::isUrlBlocked('https://' . $peer)) {
Quix0r marked this conversation as resolved.
Show resolved Hide resolved
// Ignore blocked systems as soon as possible in the loop to avoid being slowed down by tar pits
continue;
}

++$total;
if (DBA::exists('gserver', ['nurl' => Strings::normaliseLink('http://' . $peer)])) {
if (DBA::exists('gserver', ['nurl' => Strings::normaliseLink('https://' . $peer)])) {
Quix0r marked this conversation as resolved.
Show resolved Hide resolved
// We already know this server
continue;
}
Expand Down