diff --git a/CHANGELOG.md b/CHANGELOG.md index 9237d4c2..d563441e 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -25,6 +25,7 @@ The format is based on [Keep a Changelog](https://keepachangelog.com), and this #### Fixed * [#512](https://github.com/shlinkio/shlink/issues/512) Fixed query params not being properly forwarded from short URL to long one. +* [#540](https://github.com/shlinkio/shlink/issues/540) Fixed errors thrown when creating short URLs if the original URL has an internationalized domain name and URL validation is enabled. ## 1.20.0 - 2019-11-02 diff --git a/Dockerfile b/Dockerfile index 42ba3696..eae81a5a 100644 --- a/Dockerfile +++ b/Dockerfile @@ -17,7 +17,9 @@ RUN \ # Install postgres apk add --no-cache postgresql-dev && \ docker-php-ext-install -j"$(nproc)" pdo_pgsql && \ + # Install intl apk add --no-cache icu-dev && \ + docker-php-ext-install -j"$(nproc)" intl && \ # Install zip and gd apk add --no-cache libzip-dev zlib-dev libpng-dev && \ docker-php-ext-install -j"$(nproc)" zip gd diff --git a/data/infra/php.Dockerfile b/data/infra/php.Dockerfile index 2fbfaafd..0cb74aaa 100644 --- a/data/infra/php.Dockerfile +++ b/data/infra/php.Dockerfile @@ -18,6 +18,7 @@ RUN apk add --no-cache sqlite-dev RUN docker-php-ext-install pdo_sqlite RUN apk add --no-cache icu-dev +RUN docker-php-ext-install intl RUN apk add --no-cache libzip-dev zlib-dev RUN docker-php-ext-install zip diff --git a/data/infra/swoole.Dockerfile b/data/infra/swoole.Dockerfile index 5982a42b..ea7dc230 100644 --- a/data/infra/swoole.Dockerfile +++ b/data/infra/swoole.Dockerfile @@ -19,6 +19,7 @@ RUN apk add --no-cache sqlite-dev RUN docker-php-ext-install pdo_sqlite RUN apk add --no-cache icu-dev +RUN docker-php-ext-install intl RUN apk add --no-cache libzip-dev zlib-dev RUN docker-php-ext-install zip diff --git a/module/Core/config/dependencies.config.php b/module/Core/config/dependencies.config.php index a5d22179..426d0969 100644 --- a/module/Core/config/dependencies.config.php +++ b/module/Core/config/dependencies.config.php @@ -31,6 +31,8 @@ return [ Service\Tag\TagService::class => ConfigAbstractFactory::class, Service\ShortUrl\DeleteShortUrlService::class => ConfigAbstractFactory::class, + Util\UrlValidator::class => ConfigAbstractFactory::class, + Action\RedirectAction::class => ConfigAbstractFactory::class, Action\PixelAction::class => ConfigAbstractFactory::class, Action\QrCodeAction::class => ConfigAbstractFactory::class, @@ -52,13 +54,15 @@ return [ Options\NotFoundRedirectOptions::class => ['config.not_found_redirects'], Options\UrlShortenerOptions::class => ['config.url_shortener'], - Service\UrlShortener::class => ['httpClient', 'em', Options\UrlShortenerOptions::class], + Service\UrlShortener::class => [Util\UrlValidator::class, 'em', Options\UrlShortenerOptions::class], Service\VisitsTracker::class => ['em', EventDispatcherInterface::class], Service\ShortUrlService::class => ['em'], Service\VisitService::class => ['em'], Service\Tag\TagService::class => ['em'], Service\ShortUrl\DeleteShortUrlService::class => ['em', Options\DeleteShortUrlsOptions::class], + Util\UrlValidator::class => ['httpClient'], + Action\RedirectAction::class => [ Service\UrlShortener::class, Service\VisitsTracker::class, diff --git a/module/Core/src/Service/UrlShortener.php b/module/Core/src/Service/UrlShortener.php index d286069e..6b04d63a 100644 --- a/module/Core/src/Service/UrlShortener.php +++ b/module/Core/src/Service/UrlShortener.php @@ -5,10 +5,6 @@ declare(strict_types=1); namespace Shlinkio\Shlink\Core\Service; use Doctrine\ORM\EntityManagerInterface; -use Fig\Http\Message\RequestMethodInterface; -use GuzzleHttp\ClientInterface; -use GuzzleHttp\Exception\GuzzleException; -use GuzzleHttp\RequestOptions; use Psr\Http\Message\UriInterface; use Shlinkio\Shlink\Core\Domain\Resolver\PersistenceDomainResolver; use Shlinkio\Shlink\Core\Entity\ShortUrl; @@ -20,6 +16,7 @@ use Shlinkio\Shlink\Core\Model\ShortUrlMeta; use Shlinkio\Shlink\Core\Options\UrlShortenerOptions; use Shlinkio\Shlink\Core\Repository\ShortUrlRepository; use Shlinkio\Shlink\Core\Util\TagManagerTrait; +use Shlinkio\Shlink\Core\Util\UrlValidatorInterface; use Throwable; use function array_reduce; @@ -28,16 +25,19 @@ class UrlShortener implements UrlShortenerInterface { use TagManagerTrait; - /** @var ClientInterface */ - private $httpClient; /** @var EntityManagerInterface */ private $em; /** @var UrlShortenerOptions */ private $options; + /** @var UrlValidatorInterface */ + private $urlValidator; - public function __construct(ClientInterface $httpClient, EntityManagerInterface $em, UrlShortenerOptions $options) - { - $this->httpClient = $httpClient; + public function __construct( + UrlValidatorInterface $urlValidator, + EntityManagerInterface $em, + UrlShortenerOptions $options + ) { + $this->urlValidator = $urlValidator; $this->em = $em; $this->options = $options; } @@ -60,7 +60,7 @@ class UrlShortener implements UrlShortenerInterface // If the URL validation is enabled, check that the URL actually exists if ($this->options->isUrlValidationEnabled()) { - $this->checkUrlExists($url); + $this->urlValidator->validateUrl($url); } $this->em->beginTransaction(); @@ -110,17 +110,6 @@ class UrlShortener implements UrlShortenerInterface }); } - private function checkUrlExists(string $url): void - { - try { - $this->httpClient->request(RequestMethodInterface::METHOD_GET, $url, [ - RequestOptions::ALLOW_REDIRECTS => ['max' => 15], - ]); - } catch (GuzzleException $e) { - throw InvalidUrlException::fromUrl($url, $e); - } - } - private function verifyShortCodeUniqueness(ShortUrlMeta $meta, ShortUrl $shortUrlToBeCreated): void { $shortCode = $shortUrlToBeCreated->getShortCode(); diff --git a/module/Core/src/Util/UrlValidator.php b/module/Core/src/Util/UrlValidator.php new file mode 100644 index 00000000..5a30d9d3 --- /dev/null +++ b/module/Core/src/Util/UrlValidator.php @@ -0,0 +1,53 @@ +httpClient = $httpClient; + } + + /** + * @throws InvalidUrlException + */ + public function validateUrl(string $url): void + { + // FIXME Guzzle is about to add support for this https://github.com/guzzle/guzzle/pull/2286 + // Remove custom implementation when Guzzle's PR is merged + $uri = new Uri($url); + $originalHost = $uri->getHost(); + $normalizedHost = idn_to_ascii($originalHost, IDNA_DEFAULT, INTL_IDNA_VARIANT_UTS46); + if ($originalHost !== $normalizedHost) { + $uri = $uri->withHost($normalizedHost); + } + + try { + $this->httpClient->request(self::METHOD_GET, (string) $uri, [ + RequestOptions::ALLOW_REDIRECTS => ['max' => self::MAX_REDIRECTS], + ]); + } catch (GuzzleException $e) { + throw InvalidUrlException::fromUrl($url, $e); + } + } +} diff --git a/module/Core/src/Util/UrlValidatorInterface.php b/module/Core/src/Util/UrlValidatorInterface.php new file mode 100644 index 00000000..05230605 --- /dev/null +++ b/module/Core/src/Util/UrlValidatorInterface.php @@ -0,0 +1,15 @@ +httpClient = $this->prophesize(ClientInterface::class); + $this->urlValidator = $this->prophesize(UrlValidatorInterface::class); $this->em = $this->prophesize(EntityManagerInterface::class); $conn = $this->prophesize(Connection::class); @@ -63,7 +60,7 @@ class UrlShortenerTest extends TestCase private function setUrlShortener(bool $urlValidationEnabled): void { $this->urlShortener = new UrlShortener( - $this->httpClient->reveal(), + $this->urlValidator->reveal(), $this->em->reveal(), new UrlShortenerOptions(['validate_url' => $urlValidationEnabled]) ); @@ -127,20 +124,19 @@ class UrlShortenerTest extends TestCase } /** @test */ - public function exceptionIsThrownWhenUrlDoesNotExist(): void + public function validatorIsCalledWhenUrlValidationIsEnabled(): void { $this->setUrlShortener(true); + $validateUrl = $this->urlValidator->validateUrl('http://foobar.com/12345/hello?foo=bar')->will(function () { + }); - $this->httpClient->request(Argument::cetera())->willThrow( - new ClientException('', $this->prophesize(Request::class)->reveal()) - ); - - $this->expectException(InvalidUrlException::class); $this->urlShortener->urlToShortCode( new Uri('http://foobar.com/12345/hello?foo=bar'), [], ShortUrlMeta::createEmpty() ); + + $validateUrl->shouldHaveBeenCalledOnce(); } /** @test */ diff --git a/module/Core/test/Util/UrlValidatorTest.php b/module/Core/test/Util/UrlValidatorTest.php new file mode 100644 index 00000000..331ee52f --- /dev/null +++ b/module/Core/test/Util/UrlValidatorTest.php @@ -0,0 +1,66 @@ +httpClient = $this->prophesize(ClientInterface::class); + $this->urlValidator = new UrlValidator($this->httpClient->reveal()); + } + + /** @test */ + public function exceptionIsThrownWhenUrlIsInvalid(): void + { + $request = $this->httpClient->request(Argument::cetera())->willThrow( + new ClientException('', $this->prophesize(Request::class)->reveal()) + ); + + $this->expectException(InvalidUrlException::class); + $request->shouldBeCalledOnce(); + + $this->urlValidator->validateUrl('http://foobar.com/12345/hello?foo=bar'); + } + + /** + * @test + * @dataProvider provideUrls + */ + public function expectedUrlIsCalledInOrderToVerifyProvidedUrl(string $providedUrl, string $expectedUrl): void + { + $request = $this->httpClient->request( + RequestMethodInterface::METHOD_GET, + $expectedUrl, + Argument::cetera() + )->will(function () { + }); + + $this->urlValidator->validateUrl($providedUrl); + + $request->shouldHaveBeenCalledOnce(); + } + + public function provideUrls(): iterable + { + yield 'regular domain' => ['http://foobar.com', 'http://foobar.com']; + yield 'IDN' => ['https://cédric.laubacher.io/', 'https://xn--cdric-bsa.laubacher.io/']; + } +} diff --git a/module/Rest/test-api/Action/CreateShortUrlActionTest.php b/module/Rest/test-api/Action/CreateShortUrlActionTest.php index c733be25..d9730b60 100644 --- a/module/Rest/test-api/Action/CreateShortUrlActionTest.php +++ b/module/Rest/test-api/Action/CreateShortUrlActionTest.php @@ -182,6 +182,16 @@ class CreateShortUrlActionTest extends ApiTestCase $this->assertNotEquals($firstShortCode, $secondShortCode); } + /** @test */ + public function createsNewShortUrlWithInternationalizedDomainName(): void + { + $longUrl = 'https://cédric.laubacher.io/'; + [$statusCode, ['longUrl' => $expectedLongUrl]] = $this->createShortUrl(['longUrl' => $longUrl]); + + $this->assertEquals(self::STATUS_OK, $statusCode); + $this->assertEquals($expectedLongUrl, $longUrl); + } + /** * @return array { * @var int $statusCode