OilfieldToolsNet/shlink
4
0
Fork 0
mirror of https://github.com/shlinkio/shlink.git synced 2024-12-22 15:13:59 -06:00
Code Issues Packages Projects Releases Wiki Activity

Merge pull request #2013 from acelaya-forks/feature/title-resolution-timeout

Browse Source 
Add a 5-second timeout to title resolution
This commit is contained in:
Alejandro Celaya 2024-02-18 14:21:46 +01:00 committed by GitHub
commit 1a133af141
No known key found for this signature in database
GPG Key ID: B5690EEEBB952194
9 changed files with 70 additions and 27 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

2
CHANGELOG.md
View File

@ -26,7 +26,7 @@ The format is based on [Keep a Changelog](https://keepachangelog.com), and this
* [#1908](https://github.com/shlinkio/shlink/issues/1908) Remove support for openswoole (and swoole).
### Fixed
* *Nothing*
* [#2000](https://github.com/shlinkio/shlink/issues/2000) Fix short URL creation/edition getting stuck when trying to resolve the title of a long URL which never returns a response.
## [3.7.3] - 2024-01-04

1
UPGRADE.md
View File

@ -13,6 +13,7 @@
* The short URLs `loosely` mode is no longer supported, as it was a typo. Use `loose` mode instead.
* QR codes URLs now work by default, even for short URLs that cannot be visited due to max visits or date range limitations.
If you want to keep previous behavior, pass `QR_CODE_FOR_DISABLED_SHORT_URLS=false` or the equivalent configuration option.
* Long URL title resolution is now enabled by default. You can still disable it by passing `AUTO_RESOLVE_TITLES=false` or the equivalent configuration option.
* Shlink no longer allows to opt-in for long URL verification. Long URLs are unconditionally considered correct during short URL creation/edition.
### Changes in REST API

2
composer.json
View File

@ -46,7 +46,7 @@
"shlinkio/shlink-config": "dev-main#a43b380 as 3.0",
"shlinkio/shlink-event-dispatcher": "dev-main#aa9023c as 4.0",
"shlinkio/shlink-importer": "dev-main#65a9a30 as 5.3",
"shlinkio/shlink-installer": "dev-develop#3e8d7d7 as 9.0",
"shlinkio/shlink-installer": "dev-develop#b314455 as 9.0",
"shlinkio/shlink-ip-geolocation": "dev-main#a807668 as 3.5",
"shlinkio/shlink-json": "^1.1",
"spiral/roadrunner": "^2023.3",

2
config/autoload/url-shortener.global.php
View File

@ -24,7 +24,7 @@ return (static function (): array {
'hostname' => EnvVars::DEFAULT_DOMAIN->loadFromEnv(''),
],
'default_short_codes_length' => $shortCodesLength,
'auto_resolve_titles' => (bool) EnvVars::AUTO_RESOLVE_TITLES->loadFromEnv(false),
'auto_resolve_titles' => (bool) EnvVars::AUTO_RESOLVE_TITLES->loadFromEnv(true),
'append_extra_path' => (bool) EnvVars::REDIRECT_APPEND_EXTRA_PATH->loadFromEnv(false),
'multi_segment_slugs_enabled' => (bool) EnvVars::MULTI_SEGMENT_SLUGS_ENABLED->loadFromEnv(false),
'trailing_slash_enabled' => (bool) EnvVars::SHORT_URL_TRAILING_SLASH->loadFromEnv(false),

1
config/autoload/url-shortener.local.php.dist
View File

@ -14,7 +14,6 @@ return [
default => '8000',
}),
],
'auto_resolve_titles' => true,
// 'multi_segment_slugs_enabled' => true,
// 'trailing_slash_enabled' => true,
],

21
config/test/test_config.global.php
View File

@ -7,7 +7,9 @@ namespace Shlinkio\Shlink;
use GuzzleHttp\Client;
use Laminas\ConfigAggregator\ConfigAggregator;
use Laminas\Diactoros\Response\EmptyResponse;
use Laminas\Diactoros\Response\HtmlResponse;
use Laminas\ServiceManager\Factory\InvokableFactory;
use Mezzio\Router\FastRouteRouter;
use Monolog\Level;
use Shlinkio\Shlink\Common\Logger\LoggerType;
use Shlinkio\Shlink\TestUtils\ApiTest\CoverageMiddleware;
@ -18,6 +20,7 @@ use Symfony\Component\Console\Application;
use function Laminas\Stratigility\middleware;
use function Shlinkio\Shlink\Config\env;
use function Shlinkio\Shlink\Core\ArrayUtils\contains;
use function sleep;
use function sprintf;
use const ShlinkioTest\Shlink\API_TESTS_HOST;
@ -86,6 +89,7 @@ return [
'debug' => true,
ConfigAggregator::ENABLE_CACHE => false,
FastRouteRouter::CONFIG_CACHE_ENABLED => false,
'url_shortener' => [
'domain' => [
@ -94,10 +98,12 @@ return [
],
],
'routes' => !$isApiTest ? [] : [
'routes' => [
// This route is invoked at the end of API tests, in order to dump coverage collected so far
[
'name' => 'dump_coverage',
'path' => '/api-tests/stop-coverage',
'allowed_methods' => ['GET'],
'middleware' => middleware(static function () use ($coverage, $coverageType) {
// TODO I have tried moving this block to a register_shutdown_function here, which internally checks if
// RR_MODE === 'http', but this seems to be false in CI, causing the coverage to not be generated
@ -108,7 +114,17 @@ return [
);
return new EmptyResponse();
}),
],
// This route is used to test that title resolution is skipped if the long URL times out
[
'name' => 'long_url_with_timeout',
'path' => '/api-tests/long-url-with-timeout',
'allowed_methods' => ['GET'],
'middleware' => middleware(static function () {
sleep(5); // Title resolution times out at 3 seconds
return new HtmlResponse('<title>The title</title>');
}),
],
],
@ -119,6 +135,7 @@ return [
],
],
// Disable mercure integration during E2E tests
'mercure' => [
'public_hub_url' => null,
'internal_hub_url' => null,
@ -153,7 +170,7 @@ return [
'data_fixtures' => [
'paths' => [
// TODO These are used for CLI tests too, so maybe should be somewhere else
// TODO These are used for other module's tests, so maybe should be somewhere else
__DIR__ . '/../../module/Rest/test-api/Fixtures',
],
],

8
module/Core/src/ShortUrl/Helper/ShortUrlTitleResolutionHelper.php
View File

@ -22,8 +22,8 @@ use const Shlinkio\Shlink\TITLE_TAG_VALUE;
readonly class ShortUrlTitleResolutionHelper implements ShortUrlTitleResolutionHelperInterface
{
private const MAX_REDIRECTS = 15;
private const CHROME_USER_AGENT = 'Mozilla/5.0 (X11; Linux x86_64) AppleWebKit/537.36 (KHTML, like Gecko) '
public const MAX_REDIRECTS = 15;
public const CHROME_USER_AGENT = 'Mozilla/5.0 (X11; Linux x86_64) AppleWebKit/537.36 (KHTML, like Gecko) '
. 'Chrome/121.0.0.0 Safari/537.36';
public function __construct(
@ -61,7 +61,9 @@ readonly class ShortUrlTitleResolutionHelper implements ShortUrlTitleResolutionH
{
try {
return $this->httpClient->request(RequestMethodInterface::METHOD_GET, $url, [
// TODO Add a sensible timeout that prevents hanging here forever
// Add a sensible 3-second timeout that prevents hanging here forever
RequestOptions::TIMEOUT => 3,
RequestOptions::CONNECT_TIMEOUT => 3,
// Prevent potential infinite redirection loops
RequestOptions::ALLOW_REDIRECTS => ['max' => self::MAX_REDIRECTS],
RequestOptions::IDN_CONVERSION => true,

49
module/Core/test/ShortUrl/Helper/ShortUrlTitleResolutionHelperTest.php
View File

@ -5,11 +5,14 @@ declare(strict_types=1);
namespace ShlinkioTest\Shlink\Core\ShortUrl\Helper;
use Exception;
use Fig\Http\Message\RequestMethodInterface;
use GuzzleHttp\ClientInterface;
use GuzzleHttp\RequestOptions;
use Laminas\Diactoros\Response;
use Laminas\Diactoros\Response\JsonResponse;
use Laminas\Diactoros\Stream;
use PHPUnit\Framework\Attributes\Test;
use PHPUnit\Framework\MockObject\Builder\InvocationMocker;
use PHPUnit\Framework\MockObject\MockObject;
use PHPUnit\Framework\TestCase;
use Shlinkio\Shlink\Core\Options\UrlShortenerOptions;
@ -18,6 +21,8 @@ use Shlinkio\Shlink\Core\ShortUrl\Model\ShortUrlCreation;
class ShortUrlTitleResolutionHelperTest extends TestCase
{
private const LONG_URL = 'http://foobar.com/12345/hello?foo=bar';
private MockObject & ClientInterface $httpClient;
protected function setUp(): void
@ -28,7 +33,7 @@ class ShortUrlTitleResolutionHelperTest extends TestCase
#[Test]
public function dataIsReturnedAsIsWhenResolvingTitlesIsDisabled(): void
{
$data = ShortUrlCreation::fromRawData(['longUrl' => 'http://foobar.com/12345/hello?foo=bar']);
$data = ShortUrlCreation::fromRawData(['longUrl' => self::LONG_URL]);
$this->httpClient->expects($this->never())->method('request');
$result = $this->helper()->processTitle($data);
@ -40,7 +45,7 @@ class ShortUrlTitleResolutionHelperTest extends TestCase
public function dataIsReturnedAsIsWhenItAlreadyHasTitle(): void
{
$data = ShortUrlCreation::fromRawData([
'longUrl' => 'http://foobar.com/12345/hello?foo=bar',
'longUrl' => self::LONG_URL,
'title' => 'foo',
]);
$this->httpClient->expects($this->never())->method('request');
@ -53,10 +58,8 @@ class ShortUrlTitleResolutionHelperTest extends TestCase
#[Test]
public function dataIsReturnedAsIsWhenFetchingFails(): void
{
$data = ShortUrlCreation::fromRawData([
'longUrl' => 'http://foobar.com/12345/hello?foo=bar',
]);
$this->httpClient->expects($this->once())->method('request')->willThrowException(new Exception('Error'));
$data = ShortUrlCreation::fromRawData(['longUrl' => self::LONG_URL]);
$this->expectRequestToBeCalled()->willThrowException(new Exception('Error'));
$result = $this->helper(autoResolveTitles: true)->processTitle($data);
@ -66,10 +69,8 @@ class ShortUrlTitleResolutionHelperTest extends TestCase
#[Test]
public function dataIsReturnedAsIsWhenResponseIsNotHtml(): void
{
$data = ShortUrlCreation::fromRawData([
'longUrl' => 'http://foobar.com/12345/hello?foo=bar',
]);
$this->httpClient->expects($this->once())->method('request')->willReturn(new JsonResponse(['foo' => 'bar']));
$data = ShortUrlCreation::fromRawData(['longUrl' => self::LONG_URL]);
$this->expectRequestToBeCalled()->willReturn(new JsonResponse(['foo' => 'bar']));
$result = $this->helper(autoResolveTitles: true)->processTitle($data);
@ -79,10 +80,8 @@ class ShortUrlTitleResolutionHelperTest extends TestCase
#[Test]
public function dataIsReturnedAsIsWhenTitleCannotBeResolvedFromResponse(): void
{
$data = ShortUrlCreation::fromRawData([
'longUrl' => 'http://foobar.com/12345/hello?foo=bar',
]);
$this->httpClient->expects($this->once())->method('request')->willReturn($this->respWithoutTitle());
$data = ShortUrlCreation::fromRawData(['longUrl' => self::LONG_URL]);
$this->expectRequestToBeCalled()->willReturn($this->respWithoutTitle());
$result = $this->helper(autoResolveTitles: true)->processTitle($data);
@ -92,10 +91,8 @@ class ShortUrlTitleResolutionHelperTest extends TestCase
#[Test]
public function titleIsUpdatedWhenItCanBeResolvedFromResponse(): void
{
$data = ShortUrlCreation::fromRawData([
'longUrl' => 'http://foobar.com/12345/hello?foo=bar',
]);
$this->httpClient->expects($this->once())->method('request')->willReturn($this->respWithTitle());
$data = ShortUrlCreation::fromRawData(['longUrl' => self::LONG_URL]);
$this->expectRequestToBeCalled()->willReturn($this->respWithTitle());
$result = $this->helper(autoResolveTitles: true)->processTitle($data);
@ -103,6 +100,22 @@ class ShortUrlTitleResolutionHelperTest extends TestCase
self::assertEquals('Resolved "title"', $result->title);
}
private function expectRequestToBeCalled(): InvocationMocker
{
return $this->httpClient->expects($this->once())->method('request')->with(
RequestMethodInterface::METHOD_GET,
self::LONG_URL,
[
RequestOptions::TIMEOUT => 3,
RequestOptions::CONNECT_TIMEOUT => 3,
RequestOptions::ALLOW_REDIRECTS => ['max' => ShortUrlTitleResolutionHelper::MAX_REDIRECTS],
RequestOptions::IDN_CONVERSION => true,
RequestOptions::HEADERS => ['User-Agent' => ShortUrlTitleResolutionHelper::CHROME_USER_AGENT],
RequestOptions::STREAM => true,
],
);
}
private function respWithoutTitle(): Response
{
$body = $this->createStreamWithContent('<body>No title</body>');

11
module/Rest/test-api/Action/CreateShortUrlTest.php
View File

@ -328,6 +328,17 @@ class CreateShortUrlTest extends ApiTestCase
self::assertEquals('https://github.com/shlinkio/shlink/android', $payload['deviceLongUrls']['android'] ?? null);
}
#[Test]
public function titleIsIgnoredIfLongUrlTimesOut(): void
{
[$statusCode, $payload] = $this->createShortUrl([
'longUrl' => 'http://127.0.0.1:9999/api-tests/long-url-with-timeout',
]);
self::assertEquals(self::STATUS_OK, $statusCode);
self::assertNull($payload['title']);
}
/**
* @return array{int, array}
*/