Merge pull request #547 from acelaya-forks/feature/support-idn

Feature/support idn

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

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

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

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

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,

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();

module/Core/src/Util/UrlValidator.php

@@ -0,0 +1,53 @@
+<?php
+
+declare(strict_types=1);
+
+namespace Shlinkio\Shlink\Core\Util;
+
+use Fig\Http\Message\RequestMethodInterface;
+use GuzzleHttp\ClientInterface;
+use GuzzleHttp\Exception\GuzzleException;
+use GuzzleHttp\RequestOptions;
+use Shlinkio\Shlink\Core\Exception\InvalidUrlException;
+use Zend\Diactoros\Uri;
+
+use function idn_to_ascii;
+
+use const IDNA_DEFAULT;
+use const INTL_IDNA_VARIANT_UTS46;
+
+class UrlValidator implements UrlValidatorInterface, RequestMethodInterface
+{
+    private const MAX_REDIRECTS = 15;
+
+    /** @var ClientInterface */
+    private $httpClient;
+
+    public function __construct(ClientInterface $httpClient)
+    {
+        $this->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);
+        }
+    }
+}

module/Core/src/Util/UrlValidatorInterface.php

@@ -0,0 +1,15 @@
+<?php
+
+declare(strict_types=1);
+
+namespace Shlinkio\Shlink\Core\Util;
+
+use Shlinkio\Shlink\Core\Exception\InvalidUrlException;
+
+interface UrlValidatorInterface
+{
+    /**
+     * @throws InvalidUrlException
+     */
+    public function validateUrl(string $url): void;
+}

module/Core/test/Service/UrlShortenerTest.php

@@ -9,21 +9,18 @@ use Doctrine\Common\Collections\ArrayCollection;
 use Doctrine\DBAL\Connection;
 use Doctrine\ORM\EntityManagerInterface;
 use Doctrine\ORM\ORMException;
-use GuzzleHttp\ClientInterface;
-use GuzzleHttp\Exception\ClientException;
-use GuzzleHttp\Psr7\Request;
 use PHPUnit\Framework\TestCase;
 use Prophecy\Argument;
 use Prophecy\Prophecy\ObjectProphecy;
 use Shlinkio\Shlink\Core\Entity\ShortUrl;
 use Shlinkio\Shlink\Core\Entity\Tag;
-use Shlinkio\Shlink\Core\Exception\InvalidUrlException;
 use Shlinkio\Shlink\Core\Exception\NonUniqueSlugException;
 use Shlinkio\Shlink\Core\Model\ShortUrlMeta;
 use Shlinkio\Shlink\Core\Options\UrlShortenerOptions;
 use Shlinkio\Shlink\Core\Repository\ShortUrlRepository;
 use Shlinkio\Shlink\Core\Repository\ShortUrlRepositoryInterface;
 use Shlinkio\Shlink\Core\Service\UrlShortener;
+use Shlinkio\Shlink\Core\Util\UrlValidatorInterface;
 use Zend\Diactoros\Uri;
 
 use function array_map;
@@ -35,11 +32,11 @@ class UrlShortenerTest extends TestCase
     /** @var ObjectProphecy */
     private $em;
     /** @var ObjectProphecy */
-    private $httpClient;
+    private $urlValidator;
 
     public function setUp(): void
     {
-        $this->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 */

module/Core/test/Util/UrlValidatorTest.php

@@ -0,0 +1,66 @@
+<?php
+
+declare(strict_types=1);
+
+namespace ShlinkioTest\Shlink\Core\Util;
+
+use Fig\Http\Message\RequestMethodInterface;
+use GuzzleHttp\ClientInterface;
+use GuzzleHttp\Exception\ClientException;
+use PHPUnit\Framework\TestCase;
+use Prophecy\Argument;
+use Prophecy\Prophecy\ObjectProphecy;
+use Shlinkio\Shlink\Core\Exception\InvalidUrlException;
+use Shlinkio\Shlink\Core\Util\UrlValidator;
+use Zend\Diactoros\Request;
+
+class UrlValidatorTest extends TestCase
+{
+    /** @var UrlValidator */
+    private $urlValidator;
+    /** @var ObjectProphecy */
+    private $httpClient;
+
+    public function setUp(): void
+    {
+        $this->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/'];
+    }
+}

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