diff --git a/module/CLI/src/Command/ShortUrl/GenerateShortUrlCommand.php b/module/CLI/src/Command/ShortUrl/GenerateShortUrlCommand.php index 1b6a1b20..9d9c464b 100644 --- a/module/CLI/src/Command/ShortUrl/GenerateShortUrlCommand.php +++ b/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); } diff --git a/module/Core/src/Util/UrlValidator.php b/module/Core/src/Util/UrlValidator.php index 5a30d9d3..a3ffe0d8 100644 --- a/module/Core/src/Util/UrlValidator.php +++ b/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); + } } diff --git a/module/Core/test/Util/UrlValidatorTest.php b/module/Core/test/Util/UrlValidatorTest.php index 331ee52f..42e1e80e 100644 --- a/module/Core/test/Util/UrlValidatorTest.php +++ b/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); + } } diff --git a/module/Rest/test-api/Action/CreateShortUrlActionTest.php b/module/Rest/test-api/Action/CreateShortUrlActionTest.php index d9730b60..51564164 100644 --- a/module/Rest/test-api/Action/CreateShortUrlActionTest.php +++ b/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