From 994a28f31da54e6e718b5bd0433529ce2dce0f1b Mon Sep 17 00:00:00 2001 From: Alejandro Celaya Date: Sun, 3 Oct 2021 16:45:13 +0200 Subject: [PATCH] Ensured NotFoundRedirectResolver replaces placeholders from the URL --- module/Core/config/dependencies.config.php | 2 +- .../src/Config/NotFoundRedirectResolver.php | 28 ++++++-- .../Config/NotFoundRedirectResolverTest.php | 64 +++++++++++++++---- 3 files changed, 72 insertions(+), 22 deletions(-) diff --git a/module/Core/config/dependencies.config.php b/module/Core/config/dependencies.config.php index 66d854c3..7c3d7468 100644 --- a/module/Core/config/dependencies.config.php +++ b/module/Core/config/dependencies.config.php @@ -127,7 +127,7 @@ return [ Util\DoctrineBatchHelper::class => ['em'], Util\RedirectResponseHelper::class => [Options\UrlShortenerOptions::class], - Config\NotFoundRedirectResolver::class => [Util\RedirectResponseHelper::class], + Config\NotFoundRedirectResolver::class => [Util\RedirectResponseHelper::class, 'Logger_Shlink'], Action\RedirectAction::class => [ Service\ShortUrl\ShortUrlResolver::class, diff --git a/module/Core/src/Config/NotFoundRedirectResolver.php b/module/Core/src/Config/NotFoundRedirectResolver.php index 1d1d4519..531254f7 100644 --- a/module/Core/src/Config/NotFoundRedirectResolver.php +++ b/module/Core/src/Config/NotFoundRedirectResolver.php @@ -4,9 +4,11 @@ declare(strict_types=1); namespace Shlinkio\Shlink\Core\Config; +use League\Uri\Exceptions\SyntaxError; use League\Uri\Uri; use Psr\Http\Message\ResponseInterface; use Psr\Http\Message\UriInterface; +use Psr\Log\LoggerInterface; use Shlinkio\Shlink\Core\ErrorHandler\Model\NotFoundType; use Shlinkio\Shlink\Core\Util\RedirectResponseHelperInterface; @@ -18,8 +20,10 @@ class NotFoundRedirectResolver implements NotFoundRedirectResolverInterface private const DOMAIN_PLACEHOLDER = '{DOMAIN}'; private const ORIGINAL_PATH_PLACEHOLDER = '{ORIGINAL_PATH}'; - public function __construct(private RedirectResponseHelperInterface $redirectResponseHelper) - { + public function __construct( + private RedirectResponseHelperInterface $redirectResponseHelper, + private LoggerInterface $logger, + ) { } public function resolveRedirectResponse( @@ -48,13 +52,23 @@ class NotFoundRedirectResolver implements NotFoundRedirectResolverInterface { $domain = $currentUri->getAuthority(); $path = $currentUri->getPath(); - $redirectUri = Uri::createFromString($redirectUrl); + try { + $redirectUri = Uri::createFromString($redirectUrl); + } catch (SyntaxError $e) { + $this->logger->warning('It was not possible to parse "{url}" as a valid URL: {e}', [ + 'e' => $e, + 'url' => $redirectUrl, + ]); + return $redirectUrl; + } + + $replacePlaceholderForPattern = static fn (string $pattern, string $replace, callable $modifier) => + static fn (?string $value) => + $value === null ? null : str_replace($modifier($pattern), $modifier($replace), $value); $replacePlaceholders = static fn (callable $modifier) => compose( - static fn (?string $value) => - $value === null ? null : str_replace(self::DOMAIN_PLACEHOLDER, $modifier($domain), $value), - static fn (?string $value) => - $value === null ? null : str_replace(self::ORIGINAL_PATH_PLACEHOLDER, $modifier($path), $value), + $replacePlaceholderForPattern(self::DOMAIN_PLACEHOLDER, $domain, $modifier), + $replacePlaceholderForPattern(self::ORIGINAL_PATH_PLACEHOLDER, $path, $modifier), ); $replacePlaceholdersInPath = $replacePlaceholders('\Functional\id'); $replacePlaceholdersInQuery = $replacePlaceholders('\urlencode'); diff --git a/module/Core/test/Config/NotFoundRedirectResolverTest.php b/module/Core/test/Config/NotFoundRedirectResolverTest.php index 3a4c6476..0dc25768 100644 --- a/module/Core/test/Config/NotFoundRedirectResolverTest.php +++ b/module/Core/test/Config/NotFoundRedirectResolverTest.php @@ -14,9 +14,10 @@ use Prophecy\Argument; use Prophecy\PhpUnit\ProphecyTrait; use Prophecy\Prophecy\ObjectProphecy; use Psr\Http\Message\ServerRequestInterface; +use Psr\Http\Message\UriInterface; use Psr\Http\Server\MiddlewareInterface; +use Psr\Log\NullLogger; use Shlinkio\Shlink\Core\Action\RedirectAction; -use Shlinkio\Shlink\Core\Config\NotFoundRedirectConfigInterface; use Shlinkio\Shlink\Core\Config\NotFoundRedirectResolver; use Shlinkio\Shlink\Core\ErrorHandler\Model\NotFoundType; use Shlinkio\Shlink\Core\Options\NotFoundRedirectOptions; @@ -28,18 +29,11 @@ class NotFoundRedirectResolverTest extends TestCase private NotFoundRedirectResolver $resolver; private ObjectProphecy $helper; - private NotFoundRedirectConfigInterface $config; protected function setUp(): void { $this->helper = $this->prophesize(RedirectResponseHelperInterface::class); - $this->resolver = new NotFoundRedirectResolver($this->helper->reveal()); - - $this->config = new NotFoundRedirectOptions([ - 'invalidShortUrl' => 'invalidShortUrl', - 'regular404' => 'regular404', - 'baseUrl' => 'baseUrl', - ]); + $this->resolver = new NotFoundRedirectResolver($this->helper->reveal(), new NullLogger()); } /** @@ -47,13 +41,15 @@ class NotFoundRedirectResolverTest extends TestCase * @dataProvider provideRedirects */ public function expectedRedirectionIsReturnedDependingOnTheCase( + UriInterface $uri, NotFoundType $notFoundType, + NotFoundRedirectOptions $redirectConfig, string $expectedRedirectTo, ): void { $expectedResp = new Response(); $buildResp = $this->helper->buildRedirectResponse($expectedRedirectTo)->willReturn($expectedResp); - $resp = $this->resolver->resolveRedirectResponse($notFoundType, $this->config, new Uri()); + $resp = $this->resolver->resolveRedirectResponse($notFoundType, $redirectConfig, $uri); self::assertSame($expectedResp, $resp); $buildResp->shouldHaveBeenCalledOnce(); @@ -62,21 +58,61 @@ class NotFoundRedirectResolverTest extends TestCase public function provideRedirects(): iterable { yield 'base URL with trailing slash' => [ - $this->notFoundType(ServerRequestFactory::fromGlobals()->withUri(new Uri('/'))), + $uri = new Uri('/'), + $this->notFoundType(ServerRequestFactory::fromGlobals()->withUri($uri)), + new NotFoundRedirectOptions(['baseUrl' => 'baseUrl']), 'baseUrl', ]; + yield 'base URL with domain placeholder' => [ + $uri = new Uri('https://doma.in'), + $this->notFoundType(ServerRequestFactory::fromGlobals()->withUri($uri)), + new NotFoundRedirectOptions(['baseUrl' => 'https://redirect-here.com/{DOMAIN}']), + 'https://redirect-here.com/doma.in', + ]; + yield 'base URL with domain placeholder in query' => [ + $uri = new Uri('https://doma.in'), + $this->notFoundType(ServerRequestFactory::fromGlobals()->withUri($uri)), + new NotFoundRedirectOptions(['baseUrl' => 'https://redirect-here.com/?domain={DOMAIN}']), + 'https://redirect-here.com/?domain=doma.in', + ]; yield 'base URL without trailing slash' => [ - $this->notFoundType(ServerRequestFactory::fromGlobals()->withUri(new Uri(''))), + $uri = new Uri(''), + $this->notFoundType(ServerRequestFactory::fromGlobals()->withUri($uri)), + new NotFoundRedirectOptions(['baseUrl' => 'baseUrl']), 'baseUrl', ]; yield 'regular 404' => [ - $this->notFoundType(ServerRequestFactory::fromGlobals()->withUri(new Uri('/foo/bar'))), + $uri = new Uri('/foo/bar'), + $this->notFoundType(ServerRequestFactory::fromGlobals()->withUri($uri)), + new NotFoundRedirectOptions(['regular404' => 'regular404']), 'regular404', ]; + yield 'regular 404 with path placeholder in query' => [ + $uri = new Uri('/foo/bar'), + $this->notFoundType(ServerRequestFactory::fromGlobals()->withUri($uri)), + new NotFoundRedirectOptions(['regular404' => 'https://redirect-here.com/?path={ORIGINAL_PATH}']), + 'https://redirect-here.com/?path=%2Ffoo%2Fbar', + ]; + yield 'regular 404 with multiple placeholders' => [ + $uri = new Uri('https://doma.in/foo/bar'), + $this->notFoundType(ServerRequestFactory::fromGlobals()->withUri($uri)), + new NotFoundRedirectOptions([ + 'regular404' => 'https://redirect-here.com/{ORIGINAL_PATH}/{DOMAIN}/?d={DOMAIN}&p={ORIGINAL_PATH}', + ]), + 'https://redirect-here.com//foo/bar/doma.in/?d=doma.in&p=%2Ffoo%2Fbar', // TODO Fix duplicated slash + ]; yield 'invalid short URL' => [ + new Uri('/foo'), $this->notFoundType($this->requestForRoute(RedirectAction::class)), + new NotFoundRedirectOptions(['invalidShortUrl' => 'invalidShortUrl']), 'invalidShortUrl', ]; + yield 'invalid short URL with path placeholder' => [ + new Uri('/foo'), + $this->notFoundType($this->requestForRoute(RedirectAction::class)), + new NotFoundRedirectOptions(['invalidShortUrl' => 'https://redirect-here.com/{ORIGINAL_PATH}']), + 'https://redirect-here.com//foo', // TODO Fix duplicated slash + ]; } /** @test */ @@ -84,7 +120,7 @@ class NotFoundRedirectResolverTest extends TestCase { $notFoundType = $this->notFoundType($this->requestForRoute('foo')); - $result = $this->resolver->resolveRedirectResponse($notFoundType, $this->config, new Uri()); + $result = $this->resolver->resolveRedirectResponse($notFoundType, new NotFoundRedirectOptions(), new Uri()); self::assertNull($result); $this->helper->buildRedirectResponse(Argument::cetera())->shouldNotHaveBeenCalled();