IBX-12127: Fixed valid URLs being marked as invalid by ibexa:check-urls - #809
IBX-12127: Fixed valid URLs being marked as invalid by ibexa:check-urls#809tbialcz wants to merge 1 commit into
Conversation
tbialcz
added
Bug
5b182e3 to
9149e7e
Compare
9149e7e to
3fc0d75
Compare
|
alongosz
left a comment
alongosz
left a comment
There was a problem hiding this comment.
My general remark - if we're revamping this, can we use something more modern than reinvented-wheel-curl?
Like Symfony HTTP client? Or it doesn't make sense here or it's too much work? Just curious.
Remarks to the current solution, which is still a lot of great work 💪
| ignore_certificate: false | ||
| method: HEAD | ||
| fallback_to_get: true | ||
| user_agent: 'Mozilla/5.0 (X11; Linux x86_64; rv:128.0) Gecko/20100101 Firefox/128.0' |
There was a problem hiding this comment.
Shouldn't this be something more reflecting reality?
| user_agent: 'Mozilla/5.0 (X11; Linux x86_64; rv:128.0) Gecko/20100101 Firefox/128.0' | |
| user_agent: 'Ibexa DXP URL checker' |
Or does Cloudfare treat it as suspicious and it's on purpose like that?
| ]; | ||
|
|
||
| /** | ||
| * {@inheritdoc} |
There was a problem hiding this comment.
While at it, drop this too, please.
| * Based on https://www.onlineaspect.com/2009/01/26/how-to-use-curl_multi-without-blocking/ | ||
| */ | ||
| public function validate(array $urls) | ||
| public function validate(array $urls): void |
There was a problem hiding this comment.
Given the refactoring is within the scope of this task, this SonarCloud comment is valid:
Refactor this function to reduce its Cognitive Complexity from 17 to the 15 allowed.
The method is still unreadable ;-)
Perhaps some parts of it can be extracted into more atomic methods?
| } else { | ||
| // Abort on the first body chunk - the final (post-redirect) status code is already known | ||
| // and the body must not be streamed to the output (CURLOPT_RETURNTRANSFER is disabled). | ||
| curl_setopt($handler, CURLOPT_WRITEFUNCTION, static function ($handler, string $data): int { |
There was a problem hiding this comment.
Unused $handler and $data arguments can be probably removed, per SonarCloud suggestion.
| /** @var \Ibexa\Contracts\Core\Repository\URLService|\PHPUnit\Framework\MockObject\MockObject */ | ||
| private $urlService; |
There was a problem hiding this comment.
Type hint can be formed better here. It's never this or that in case of a mocked type. It's always an intersection. Moreover for PHP 7.4 we explicitly specify the essential/basic type and provide more details in PHPDoc. For 5.0 this can be pure PHP 8 intersection.
So:
| /** @var \Ibexa\Contracts\Core\Repository\URLService|\PHPUnit\Framework\MockObject\MockObject */ | |
| private $urlService; | |
| /** @var \Ibexa\Contracts\Core\Repository\URLService & \PHPUnit\Framework\MockObject\MockObject */ | |
| private URLService $urlService; |
| /** @var \Ibexa\Contracts\Core\SiteAccess\ConfigResolverInterface|\PHPUnit\Framework\MockObject\MockObject */ | ||
| private $configResolver; |
| message: '#^Parameter \#2 \$handle of function curl_multi_add_handle expects CurlHandle, resource given\.$#' | ||
| identifier: argument.type | ||
| count: 2 | ||
| count: 3 |
There was a problem hiding this comment.
This looks like a regression. Count 2->3.
2 participants
Description:
ibexa:check-urlschecked links with a bare cURLHEADrequest without any headers (since the original implementation, EZP-28505). WAFs like Cloudflare block such requests, so working links were marked as invalid.Now the request sends browser-like
User-Agent/Acceptheaders, and a failedHEADis retried once withGET. New optional handler options:method,fallback_to_get,user_agent,headers.Also fixed in the same code: the
httpshandler was readinghttpoptions, and thecurl_multiloop could skip trailing URLs.For QA:
https://www.tiendanimal.es/and to a URL that rejectsHEADrequests (e.g. a localphp -Sserver returning 403 forHEAD), publish.php bin/console ibexa:check-urls— both links are Valid in Link manager (before the fix: Invalid).Documentation:
Yes — document the new
url_checker.handlers.http|httpsoptions:method,fallback_to_get,user_agent,headers.