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

Catch ValueError coming from favicon lib in case of invalid redirects #2054

Closed
wants to merge 2 commits into from
Closed

Catch ValueError coming from favicon lib in case of invalid redirects #2054

Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
d030be4
Catch ValueError coming from favicon lib in case of invalid redirects
Grotax Jan 11, 2023
d02494e
add changelog entry
Grotax Jan 11, 2023
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
42 changes: 33 additions & 9 deletions lib/Fetcher/FeedFetcher.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -344,6 +344,30 @@ protected function buildItem(
return $item;
}

/**
* Helper function for fetching favicon urls
*
* @param string $base_url Base URL for the feed
*
* @return string|mixed|bool
Grotax marked this conversation as resolved.
Show resolved Hide resolved
*/
protected function faviconHelper(string $base_url)
{
try {
return $this->faviconFactory->get($base_url);
} catch (\Error $e) {
//Actual error should be ValueError but that is only implemented in PHP 8+
Copy link
Contributor

Choose a reason for hiding this comment

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

If it's only in PHP 8+, how can the factory use it? Aren't they using their own ValueError? Because otherwise it can't throw it either.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yea I think it's not their own ValueError, the lib doesn't define any Exceptions if I'm not wrong instead this error from the issue came from PHP itself. Which I guess depends on your PHP version.

The docs for the function that is throwing the error did not inlcude further information.

This should catch the error in php 8+ which the report was also based on and below that worst case the error is not caught because they used something else..

Copy link
Contributor

Choose a reason for hiding this comment

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

Can we reproduce it somehow so I can trace and update it in the lib?

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 don't know, the linked issue did have some hints on how it probably happens but for that you would need to miss misconfigure a Webserver on purpose, with an invalid redirect for the favicon.

Copy link
Contributor

Choose a reason for hiding this comment

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

It would be easier if we just find the feed where this happens. If we don't make this library more resiliant to errors we'll keep running into this.

Copy link
Member Author

Choose a reason for hiding this comment

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

True but the reporter was apparently not able to name the affected feed. The feed that was mentioned there did not cause that error.

$this->logger->info(
'An error occurred while trying to download the favicon of {url}: {error}',
[
'url' => $base_url,
'error' => $e->getMessage() ?? 'Unknown'
]
);
return false;
}
}

/**
* Return the favicon for a given feed and url
*
Expand All @@ -357,20 +381,20 @@ protected function getFavicon(FeedInterface $feed, string $url)
$favicon = null;
// trim the string because authors do funny things
$feed_logo = $feed->getLogo();

if (!is_null($feed_logo)) {
$favicon = trim($feed_logo);
}


ini_set('user_agent', 'NextCloud-News/1.0');

$base_url = new Net_URL2($url);
$base_url->setPath("");
$base_url = $base_url->getNormalizedURL();


if (!is_null($feed_logo)) {
$favicon = trim($feed_logo);
}

// check if feed has a logo entry
if (is_null($favicon) || $favicon === '') {
return $this->faviconFactory->get($base_url);
return $this->faviconHelper($base_url);
}

// logo will be saved in the tmp folder provided by Nextcloud, file is named as md5 of the url
Expand Down Expand Up @@ -422,13 +446,13 @@ protected function getFavicon(FeedInterface $feed, string $url)

// check if file is actually an image
if (!$is_image) {
return $this->faviconFactory->get($base_url);
return $this->faviconHelper($base_url);
}

list($width, $height, $type, $attr) = getimagesize($favicon_path);
// check if image is square else fall back to favicon
if ($width !== $height) {
return $this->faviconFactory->get($base_url);
return $this->faviconHelper($base_url);
}

return $favicon;
Expand Down