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

Add additional checks for banned instances + various code improvements along the way #1258

Merged
merged 19 commits into from
Dec 31, 2024
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
19 commits
Select commit Hold shift + click to select a range
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
24 changes: 22 additions & 2 deletions src/Service/ActivityPub/ApHttpClient.php
Original file line number Diff line number Diff line change
Expand Up @@ -205,6 +205,11 @@ private function getActorCacheKey(string $apProfileId): string
return 'ap_'.hash('sha256', $apProfileId);
}

private function getCollectinCacheKey(string $apAddress): string
melroy89 marked this conversation as resolved.
Show resolved Hide resolved
{
return 'ap_collection'.hash('sha256', $apAddress);
}

/**
* Retrieve AP actor object (could be a user or magazine).
*
Expand Down Expand Up @@ -280,22 +285,37 @@ private function getActorObjectImpl(string $apProfileId): ?string
return $response->getContent(false);
}

/**
* Remove actor object from cache.
*
* @param string $apProfileId AP profile ID to remove from cache
*/
public function invalidateActorObjectCache(string $apProfileId): void
{
$this->cache->delete($this->getActorCacheKey($apProfileId));
}

/**
* Remove collection object from cache.
*
* @param string $apAddress AP address to remove from cache
*/
public function invalidateCollectionObjectCache(string $apAddress): void
{
$this->cache->delete('ap_collection'.hash('sha256', $apAddress));
$this->cache->delete($this->getCollectinCacheKey($apAddress));
}

/**
* Retrieve AP collection object. First look in cache, then try to retrieve from AP server.
* And finally, save the response to cache.
*
* @return Response body
melroy89 marked this conversation as resolved.
Show resolved Hide resolved
*
* @throws InvalidArgumentException
*/
public function getCollectionObject(string $apAddress): ?array
{
$key = 'ap_collection'.hash('sha256', $apAddress);
$key = $this->getCollectinCacheKey($apAddress);
if ($this->cache->hasItem($key)) {
/** @var CacheItem $item */
$item = $this->cache->getItem($key);
Expand Down
20 changes: 18 additions & 2 deletions src/Service/ActivityPubManager.php
Original file line number Diff line number Diff line change
Expand Up @@ -126,7 +126,6 @@ public function createCcFromBody(string $body): array
*
* @return User|Magazine|null or Magazine or null on error
*
* @throws InstanceBannedException
* @throws InvalidApPostException
* @throws InvalidArgumentException
* @throws InvalidWebfingerException
Expand Down Expand Up @@ -161,6 +160,11 @@ public function findActorOrCreate(?string $actorUrlOrHandle): User|Magazine|null
$actorUrl = $this->webfinger($actorUrl)->getProfileId();
}

// Check if the instance is banned
if ($this->settingsManager->isBannedInstance($actorUrl)) {
return null;
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This check should be pushed down to the line just before $actor = $this->apHttpClient->getActorObject($actorUrl);

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are you sure? This might still do dispatchUpdateActor.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

dispatch update actor checks for banned instances as well so this is not an issue i think

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes indeed, it checks it:

if ($this->settingsManager->isBannedInstance($actorUrl)) {
. But not dispatching in the first place is more efficient, which I like. And reduce unnecessary CPU cycles etc.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well it does the check before actually dispatching the update:

public function dispatchUpdateActor(string $actorUrl)
{
if ($this->settingsManager->isBannedInstance($actorUrl)) {
return;
}

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I pushed the isBannedInstance check down to line 176 now.

The question now is whether you really want to return users or magazines from a banned instance..?


if (\in_array(
parse_url($actorUrl, PHP_URL_HOST),
[$this->settingsManager->get('KBIN_DOMAIN'), 'localhost', '127.0.0.1']
Expand Down Expand Up @@ -279,7 +283,7 @@ public function webfinger(string $id): WebFinger
return $this->webFingerFactory->get($handle);
}

public function buildHandle(string $id): string
private function buildHandle(string $id): string
{
$port = !\is_null(parse_url($id, PHP_URL_PORT))
? ':'.parse_url($id, PHP_URL_PORT)
Expand Down Expand Up @@ -337,6 +341,10 @@ public function updateUser(string $actorUrl): ?User
return $user;
}

if ($this->settingsManager->isBannedInstance($actorUrl)) {
return null;
}
melroy89 marked this conversation as resolved.
Show resolved Hide resolved

$actor = $this->apHttpClient->getActorObject($actorUrl);
if (!$actor || !\is_array($actor)) {
return null;
Expand Down Expand Up @@ -511,6 +519,10 @@ public function updateMagazine(string $actorUrl): ?Magazine
return $magazine;
}

if ($this->settingsManager->isBannedInstance($actorUrl)) {
return null;
}
melroy89 marked this conversation as resolved.
Show resolved Hide resolved

$actor = $this->apHttpClient->getActorObject($actorUrl);
// Check if actor isn't empty (not set/null/empty array/etc.)

Expand Down Expand Up @@ -868,6 +880,10 @@ public function handleExternalVideos(array $attachment): ?array
*/
public function updateActor(string $actorUrl): Magazine|User|null
{
if ($this->settingsManager->isBannedInstance($actorUrl)) {
return null;
}

if ($this->userRepository->findOneBy(['apProfileId' => $actorUrl])) {
return $this->updateUser($actorUrl);
} elseif ($this->magazineRepository->findOneBy(['apProfileId' => $actorUrl])) {
Expand Down
Loading