Merge pull request #548 from acelaya-forks/feature/redirect-to-idn

Handled IDN domains also on internal redirections when validating a URL

module/CLI/src/Command/ShortUrl/GenerateShortUrlCommand.php

@@ -102,7 +102,7 @@ class GenerateShortUrlCommand extends Command
             return;
         }
 
-        $longUrl = $io->ask('A long URL was not provided. Which URL do you want to be shortened?');
+        $longUrl = $io->ask('Which URL do you want to shorten?');
         if (! empty($longUrl)) {
             $input->setArgument('longUrl', $longUrl);
         }

module/Core/src/Util/UrlValidator.php

@@ -5,18 +5,20 @@ declare(strict_types=1);
 namespace Shlinkio\Shlink\Core\Util;
 
 use Fig\Http\Message\RequestMethodInterface;
+use Fig\Http\Message\StatusCodeInterface;
 use GuzzleHttp\ClientInterface;
 use GuzzleHttp\Exception\GuzzleException;
 use GuzzleHttp\RequestOptions;
 use Shlinkio\Shlink\Core\Exception\InvalidUrlException;
 use Zend\Diactoros\Uri;
 
+use function Functional\contains;
 use function idn_to_ascii;
 
 use const IDNA_DEFAULT;
 use const INTL_IDNA_VARIANT_UTS46;
 
-class UrlValidator implements UrlValidatorInterface, RequestMethodInterface
+class UrlValidator implements UrlValidatorInterface, RequestMethodInterface, StatusCodeInterface
 {
     private const MAX_REDIRECTS = 15;
 
@@ -32,9 +34,17 @@ class UrlValidator implements UrlValidatorInterface, RequestMethodInterface
      * @throws InvalidUrlException
      */
     public function validateUrl(string $url): void
+    {
+        $this->doValidateUrl($url);
+    }
+
+    /**
+     * @throws InvalidUrlException
+     */
+    private function doValidateUrl(string $url, int $redirectNum = 1): 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
+        //       Remove custom implementation and manual redirect handling when Guzzle's PR is merged
         $uri = new Uri($url);
         $originalHost = $uri->getHost();
         $normalizedHost = idn_to_ascii($originalHost, IDNA_DEFAULT, INTL_IDNA_VARIANT_UTS46);
@@ -43,11 +53,21 @@ class UrlValidator implements UrlValidatorInterface, RequestMethodInterface
         }
 
         try {
-            $this->httpClient->request(self::METHOD_GET, (string) $uri, [
-                RequestOptions::ALLOW_REDIRECTS => ['max' => self::MAX_REDIRECTS],
+            $resp = $this->httpClient->request(self::METHOD_GET, (string) $uri, [
+//                RequestOptions::ALLOW_REDIRECTS => ['max' => self::MAX_REDIRECTS],
+                RequestOptions::ALLOW_REDIRECTS => false,
             ]);
+
+            if ($redirectNum < self::MAX_REDIRECTS && $this->statusIsRedirect($resp->getStatusCode())) {
+                $this->doValidateUrl($resp->getHeaderLine('Location'), $redirectNum + 1);
+            }
         } catch (GuzzleException $e) {
             throw InvalidUrlException::fromUrl($url, $e);
         }
     }
+
+    private function statusIsRedirect(int $statusCode): bool
+    {
+        return contains([self::STATUS_MOVED_PERMANENTLY, self::STATUS_FOUND], $statusCode);
+    }
 }

module/Core/test/Util/UrlValidatorTest.php

@@ -13,6 +13,10 @@ use Prophecy\Prophecy\ObjectProphecy;
 use Shlinkio\Shlink\Core\Exception\InvalidUrlException;
 use Shlinkio\Shlink\Core\Util\UrlValidator;
 use Zend\Diactoros\Request;
+use Zend\Diactoros\Response;
+
+use function Functional\map;
+use function range;
 
 class UrlValidatorTest extends TestCase
 {
@@ -27,19 +31,39 @@ class UrlValidatorTest extends TestCase
         $this->urlValidator = new UrlValidator($this->httpClient->reveal());
     }
 
-    /** @test */
-    public function exceptionIsThrownWhenUrlIsInvalid(): void
+    /**
+     * @test
+     * @dataProvider provideAttemptThatThrows
+     */
+    public function exceptionIsThrownWhenUrlIsInvalid(int $attemptThatThrows): void
     {
-        $request = $this->httpClient->request(Argument::cetera())->willThrow(
-            new ClientException('', $this->prophesize(Request::class)->reveal())
+        $callNum = 1;
+        $e = new ClientException('', $this->prophesize(Request::class)->reveal());
+
+        $request = $this->httpClient->request(Argument::cetera())->will(
+            function () use ($e, $attemptThatThrows, &$callNum) {
+                if ($callNum === $attemptThatThrows) {
+                    throw $e;
+                }
+
+                $callNum++;
+                return new Response('php://memory', 302, ['Location' => 'http://foo.com']);
+            }
         );
 
+        $request->shouldBeCalledTimes($attemptThatThrows);
         $this->expectException(InvalidUrlException::class);
-        $request->shouldBeCalledOnce();
 
         $this->urlValidator->validateUrl('http://foobar.com/12345/hello?foo=bar');
     }
 
+    public function provideAttemptThatThrows(): iterable
+    {
+        return map(range(1, 15), function (int $attempt) {
+            return [$attempt];
+        });
+    }
+
     /**
      * @test
      * @dataProvider provideUrls
@@ -50,8 +74,7 @@ class UrlValidatorTest extends TestCase
             RequestMethodInterface::METHOD_GET,
             $expectedUrl,
             Argument::cetera()
-        )->will(function () {
-        });
+        )->willReturn(new Response());
 
         $this->urlValidator->validateUrl($providedUrl);
 
@@ -63,4 +86,16 @@ class UrlValidatorTest extends TestCase
         yield 'regular domain' => ['http://foobar.com', 'http://foobar.com'];
         yield 'IDN' => ['https://cédric.laubacher.io/', 'https://xn--cdric-bsa.laubacher.io/'];
     }
+
+    /** @test */
+    public function considersUrlValidWhenTooManyRedirectsAreReturned(): void
+    {
+        $request = $this->httpClient->request(Argument::cetera())->willReturn(
+            new Response('php://memory', 302, ['Location' => 'http://foo.com'])
+        );
+
+        $this->urlValidator->validateUrl('http://foobar.com');
+
+        $request->shouldHaveBeenCalledTimes(15);
+    }
 }

module/Rest/test-api/Action/CreateShortUrlActionTest.php

@@ -182,16 +182,24 @@ class CreateShortUrlActionTest extends ApiTestCase
         $this->assertNotEquals($firstShortCode, $secondShortCode);
     }
 
-    /** @test */
-    public function createsNewShortUrlWithInternationalizedDomainName(): void
+    /**
+     * @test
+     * @dataProvider provideIdn
+     */
+    public function createsNewShortUrlWithInternationalizedDomainName(string $longUrl): void
     {
-        $longUrl = 'https://cédric.laubacher.io/';
         [$statusCode, ['longUrl' => $expectedLongUrl]] = $this->createShortUrl(['longUrl' => $longUrl]);
 
         $this->assertEquals(self::STATUS_OK, $statusCode);
         $this->assertEquals($expectedLongUrl, $longUrl);
     }
 
+    public function provideIdn(): iterable
+    {
+        // TODO Create some shlink IDN domains to test this instead of using public ones
+        return [['https://cédric.laubacher.io/'], ['https://laubacher.io/']]; // Second one redirects to first
+    }
+
     /**
      * @return array {
      *     @var int $statusCode