From deeca582db1d9839ef5cee10e53e1f7df20829ab Mon Sep 17 00:00:00 2001 From: Alejandro Celaya Date: Tue, 10 Nov 2020 17:30:06 +0100 Subject: [PATCH 1/4] #867 Small refactoring on NotFoundRedirecthandler --- .../ErrorHandler/NotFoundRedirectHandler.php | 18 ++++++++++-------- 1 file changed, 10 insertions(+), 8 deletions(-) diff --git a/module/Core/src/ErrorHandler/NotFoundRedirectHandler.php b/module/Core/src/ErrorHandler/NotFoundRedirectHandler.php index 13615b7d..37557f4d 100644 --- a/module/Core/src/ErrorHandler/NotFoundRedirectHandler.php +++ b/module/Core/src/ErrorHandler/NotFoundRedirectHandler.php @@ -4,7 +4,7 @@ declare(strict_types=1); namespace Shlinkio\Shlink\Core\ErrorHandler; -use Laminas\Diactoros\Response; +use Laminas\Diactoros\Response\RedirectResponse; use Mezzio\Router\RouteResult; use Psr\Http\Message\ResponseInterface; use Psr\Http\Message\ServerRequestInterface; @@ -12,17 +12,19 @@ use Psr\Http\Message\UriInterface; use Psr\Http\Server\MiddlewareInterface; use Psr\Http\Server\RequestHandlerInterface; use Shlinkio\Shlink\Core\Action\RedirectAction; -use Shlinkio\Shlink\Core\Options\NotFoundRedirectOptions; +use Shlinkio\Shlink\Core\Options; use function rtrim; class NotFoundRedirectHandler implements MiddlewareInterface { - private NotFoundRedirectOptions $redirectOptions; + private Options\NotFoundRedirectOptions $redirectOptions; private string $shlinkBasePath; - public function __construct(NotFoundRedirectOptions $redirectOptions, string $shlinkBasePath) - { + public function __construct( + Options\NotFoundRedirectOptions $redirectOptions, + string $shlinkBasePath + ) { $this->redirectOptions = $redirectOptions; $this->shlinkBasePath = $shlinkBasePath; } @@ -41,11 +43,11 @@ class NotFoundRedirectHandler implements MiddlewareInterface $isBaseUrl = rtrim($uri->getPath(), '/') === $this->shlinkBasePath; if ($isBaseUrl && $this->redirectOptions->hasBaseUrlRedirect()) { - return new Response\RedirectResponse($this->redirectOptions->getBaseUrlRedirect()); + return new RedirectResponse($this->redirectOptions->getBaseUrlRedirect()); } if (!$isBaseUrl && $routeResult->isFailure() && $this->redirectOptions->hasRegular404Redirect()) { - return new Response\RedirectResponse($this->redirectOptions->getRegular404Redirect()); + return new RedirectResponse($this->redirectOptions->getRegular404Redirect()); } if ( @@ -53,7 +55,7 @@ class NotFoundRedirectHandler implements MiddlewareInterface $routeResult->getMatchedRouteName() === RedirectAction::class && $this->redirectOptions->hasInvalidShortUrlRedirect() ) { - return new Response\RedirectResponse($this->redirectOptions->getInvalidShortUrlRedirect()); + return new RedirectResponse($this->redirectOptions->getInvalidShortUrlRedirect()); } return null; From 259c52a6988e20c25665b5284b74c069c65a1dad Mon Sep 17 00:00:00 2001 From: Alejandro Celaya Date: Tue, 10 Nov 2020 18:08:25 +0100 Subject: [PATCH 2/4] #867 Ensured status code config is honored when doing not-found redirects --- module/Core/config/dependencies.config.php | 10 +++- module/Core/src/Action/RedirectAction.php | 17 ++---- .../ErrorHandler/NotFoundRedirectHandler.php | 15 +++-- .../Core/src/Util/RedirectResponseHelper.php | 32 +++++++++++ .../Util/RedirectResponseHelperInterface.php | 12 ++++ .../Core/test/Action/RedirectActionTest.php | 55 ++++--------------- .../NotFoundRedirectHandlerTest.php | 17 +++++- .../test/Util/RedirectResponseHelperTest.php | 55 +++++++++++++++++++ 8 files changed, 149 insertions(+), 64 deletions(-) create mode 100644 module/Core/src/Util/RedirectResponseHelper.php create mode 100644 module/Core/src/Util/RedirectResponseHelperInterface.php create mode 100644 module/Core/test/Util/RedirectResponseHelperTest.php diff --git a/module/Core/config/dependencies.config.php b/module/Core/config/dependencies.config.php index 5c7c0b54..b6beb1ac 100644 --- a/module/Core/config/dependencies.config.php +++ b/module/Core/config/dependencies.config.php @@ -36,6 +36,7 @@ return [ Util\UrlValidator::class => ConfigAbstractFactory::class, Util\DoctrineBatchHelper::class => ConfigAbstractFactory::class, + Util\RedirectResponseHelper::class => ConfigAbstractFactory::class, Action\RedirectAction::class => ConfigAbstractFactory::class, Action\PixelAction::class => ConfigAbstractFactory::class, @@ -54,7 +55,11 @@ return [ ], ConfigAbstractFactory::class => [ - ErrorHandler\NotFoundRedirectHandler::class => [NotFoundRedirectOptions::class, 'config.router.base_path'], + ErrorHandler\NotFoundRedirectHandler::class => [ + NotFoundRedirectOptions::class, + Util\RedirectResponseHelper::class, + 'config.router.base_path', + ], ErrorHandler\NotFoundTemplateHandler::class => [TemplateRendererInterface::class], Options\AppOptions::class => ['config.app_options'], @@ -88,12 +93,13 @@ return [ Util\UrlValidator::class => ['httpClient', Options\UrlShortenerOptions::class], Util\DoctrineBatchHelper::class => ['em'], + Util\RedirectResponseHelper::class => [Options\UrlShortenerOptions::class], Action\RedirectAction::class => [ Service\ShortUrl\ShortUrlResolver::class, Service\VisitsTracker::class, Options\AppOptions::class, - Options\UrlShortenerOptions::class, + Util\RedirectResponseHelper::class, 'Logger_Shlink', ], Action\PixelAction::class => [ diff --git a/module/Core/src/Action/RedirectAction.php b/module/Core/src/Action/RedirectAction.php index 80fef898..0fc6232d 100644 --- a/module/Core/src/Action/RedirectAction.php +++ b/module/Core/src/Action/RedirectAction.php @@ -5,7 +5,6 @@ declare(strict_types=1); namespace Shlinkio\Shlink\Core\Action; use Fig\Http\Message\StatusCodeInterface; -use Laminas\Diactoros\Response\RedirectResponse; use Psr\Http\Message\ResponseInterface as Response; use Psr\Http\Message\ServerRequestInterface; use Psr\Http\Server\RequestHandlerInterface; @@ -13,32 +12,26 @@ use Psr\Log\LoggerInterface; use Shlinkio\Shlink\Core\Options; use Shlinkio\Shlink\Core\Service\ShortUrl\ShortUrlResolverInterface; use Shlinkio\Shlink\Core\Service\VisitsTrackerInterface; - -use function sprintf; +use Shlinkio\Shlink\Core\Util\RedirectResponseHelperInterface; class RedirectAction extends AbstractTrackingAction implements StatusCodeInterface { - private Options\UrlShortenerOptions $urlShortenerOptions; + private RedirectResponseHelperInterface $redirectResponseHelper; public function __construct( ShortUrlResolverInterface $urlResolver, VisitsTrackerInterface $visitTracker, Options\AppOptions $appOptions, - Options\UrlShortenerOptions $urlShortenerOptions, + RedirectResponseHelperInterface $redirectResponseHelper, ?LoggerInterface $logger = null ) { parent::__construct($urlResolver, $visitTracker, $appOptions, $logger); - $this->urlShortenerOptions = $urlShortenerOptions; + $this->redirectResponseHelper = $redirectResponseHelper; } protected function createSuccessResp(string $longUrl): Response { - $statusCode = $this->urlShortenerOptions->redirectStatusCode(); - $headers = $statusCode === self::STATUS_FOUND ? [] : [ - 'Cache-Control' => sprintf('private,max-age=%s', $this->urlShortenerOptions->redirectCacheLifetime()), - ]; - - return new RedirectResponse($longUrl, $statusCode, $headers); + return $this->redirectResponseHelper->buildRedirectResponse($longUrl); } protected function createErrorResp(ServerRequestInterface $request, RequestHandlerInterface $handler): Response diff --git a/module/Core/src/ErrorHandler/NotFoundRedirectHandler.php b/module/Core/src/ErrorHandler/NotFoundRedirectHandler.php index 37557f4d..a49db5bb 100644 --- a/module/Core/src/ErrorHandler/NotFoundRedirectHandler.php +++ b/module/Core/src/ErrorHandler/NotFoundRedirectHandler.php @@ -4,7 +4,6 @@ declare(strict_types=1); namespace Shlinkio\Shlink\Core\ErrorHandler; -use Laminas\Diactoros\Response\RedirectResponse; use Mezzio\Router\RouteResult; use Psr\Http\Message\ResponseInterface; use Psr\Http\Message\ServerRequestInterface; @@ -13,20 +12,24 @@ use Psr\Http\Server\MiddlewareInterface; use Psr\Http\Server\RequestHandlerInterface; use Shlinkio\Shlink\Core\Action\RedirectAction; use Shlinkio\Shlink\Core\Options; +use Shlinkio\Shlink\Core\Util\RedirectResponseHelperInterface; use function rtrim; class NotFoundRedirectHandler implements MiddlewareInterface { private Options\NotFoundRedirectOptions $redirectOptions; + private RedirectResponseHelperInterface $redirectResponseHelper; private string $shlinkBasePath; public function __construct( Options\NotFoundRedirectOptions $redirectOptions, + RedirectResponseHelperInterface $redirectResponseHelper, string $shlinkBasePath ) { $this->redirectOptions = $redirectOptions; $this->shlinkBasePath = $shlinkBasePath; + $this->redirectResponseHelper = $redirectResponseHelper; } public function process(ServerRequestInterface $request, RequestHandlerInterface $handler): ResponseInterface @@ -43,11 +46,13 @@ class NotFoundRedirectHandler implements MiddlewareInterface $isBaseUrl = rtrim($uri->getPath(), '/') === $this->shlinkBasePath; if ($isBaseUrl && $this->redirectOptions->hasBaseUrlRedirect()) { - return new RedirectResponse($this->redirectOptions->getBaseUrlRedirect()); + return $this->redirectResponseHelper->buildRedirectResponse($this->redirectOptions->getBaseUrlRedirect()); } if (!$isBaseUrl && $routeResult->isFailure() && $this->redirectOptions->hasRegular404Redirect()) { - return new RedirectResponse($this->redirectOptions->getRegular404Redirect()); + return $this->redirectResponseHelper->buildRedirectResponse( + $this->redirectOptions->getRegular404Redirect(), + ); } if ( @@ -55,7 +60,9 @@ class NotFoundRedirectHandler implements MiddlewareInterface $routeResult->getMatchedRouteName() === RedirectAction::class && $this->redirectOptions->hasInvalidShortUrlRedirect() ) { - return new RedirectResponse($this->redirectOptions->getInvalidShortUrlRedirect()); + return $this->redirectResponseHelper->buildRedirectResponse( + $this->redirectOptions->getInvalidShortUrlRedirect(), + ); } return null; diff --git a/module/Core/src/Util/RedirectResponseHelper.php b/module/Core/src/Util/RedirectResponseHelper.php new file mode 100644 index 00000000..58f6e145 --- /dev/null +++ b/module/Core/src/Util/RedirectResponseHelper.php @@ -0,0 +1,32 @@ +options = $options; + } + + public function buildRedirectResponse(string $location): ResponseInterface + { + $statusCode = $this->options->redirectStatusCode(); + $headers = $statusCode === StatusCodeInterface::STATUS_FOUND ? [] : [ + 'Cache-Control' => sprintf('private,max-age=%s', $this->options->redirectCacheLifetime()), + ]; + + return new RedirectResponse($location, $statusCode, $headers); + } +} diff --git a/module/Core/src/Util/RedirectResponseHelperInterface.php b/module/Core/src/Util/RedirectResponseHelperInterface.php new file mode 100644 index 00000000..11bb943c --- /dev/null +++ b/module/Core/src/Util/RedirectResponseHelperInterface.php @@ -0,0 +1,12 @@ +urlResolver = $this->prophesize(ShortUrlResolverInterface::class); $this->visitTracker = $this->prophesize(VisitsTrackerInterface::class); - $this->shortenerOpts = new Options\UrlShortenerOptions(); + $this->redirectRespHelper = $this->prophesize(RedirectResponseHelperInterface::class); $this->action = new RedirectAction( $this->urlResolver->reveal(), $this->visitTracker->reveal(), new Options\AppOptions(['disableTrackParam' => 'foobar']), - $this->shortenerOpts, + $this->redirectRespHelper->reveal(), ); } @@ -59,14 +60,14 @@ class RedirectActionTest extends TestCase )->willReturn($shortUrl); $track = $this->visitTracker->track(Argument::cetera())->will(function (): void { }); + $expectedResp = new Response\RedirectResponse($expectedUrl); + $buildResp = $this->redirectRespHelper->buildRedirectResponse($expectedUrl)->willReturn($expectedResp); $request = (new ServerRequest())->withAttribute('shortCode', $shortCode)->withQueryParams($query); $response = $this->action->process($request, $this->prophesize(RequestHandlerInterface::class)->reveal()); - self::assertInstanceOf(Response\RedirectResponse::class, $response); - self::assertEquals(302, $response->getStatusCode()); - self::assertTrue($response->hasHeader('Location')); - self::assertEquals($expectedUrl, $response->getHeaderLine('Location')); + self::assertSame($expectedResp, $response); + $buildResp->shouldHaveBeenCalledOnce(); $shortCodeToUrl->shouldHaveBeenCalledOnce(); $track->shouldHaveBeenCalledTimes(array_key_exists('foobar', $query) ? 0 : 1); } @@ -107,6 +108,9 @@ class RedirectActionTest extends TestCase $this->urlResolver->resolveEnabledShortUrl(new ShortUrlIdentifier($shortCode, ''))->willReturn($shortUrl); $track = $this->visitTracker->track(Argument::cetera())->will(function (): void { }); + $buildResp = $this->redirectRespHelper->buildRedirectResponse( + 'http://domain.com/foo/bar?some=thing', + )->willReturn(new Response\RedirectResponse('')); $request = (new ServerRequest())->withAttribute('shortCode', $shortCode) ->withAttribute( @@ -115,42 +119,7 @@ class RedirectActionTest extends TestCase ); $this->action->process($request, $this->prophesize(RequestHandlerInterface::class)->reveal()); + $buildResp->shouldHaveBeenCalled(); $track->shouldNotHaveBeenCalled(); } - - /** - * @test - * @dataProvider provideRedirectConfigs - */ - public function expectedStatusCodeAndCacheIsReturnedBasedOnConfig( - int $configuredStatus, - int $configuredLifetime, - int $expectedStatus, - ?string $expectedCacheControl - ): void { - $this->shortenerOpts->redirectStatusCode = $configuredStatus; - $this->shortenerOpts->redirectCacheLifetime = $configuredLifetime; - - $shortUrl = new ShortUrl('http://domain.com/foo/bar'); - $shortCode = $shortUrl->getShortCode(); - $this->urlResolver->resolveEnabledShortUrl(Argument::cetera())->willReturn($shortUrl); - - $request = (new ServerRequest())->withAttribute('shortCode', $shortCode); - $response = $this->action->process($request, $this->prophesize(RequestHandlerInterface::class)->reveal()); - - self::assertInstanceOf(Response\RedirectResponse::class, $response); - self::assertEquals($expectedStatus, $response->getStatusCode()); - self::assertEquals($response->hasHeader('Cache-Control'), $expectedCacheControl !== null); - self::assertEquals($response->getHeaderLine('Cache-Control'), $expectedCacheControl ?? ''); - } - - public function provideRedirectConfigs(): iterable - { - yield 'status 302' => [302, 20, 302, null]; - yield 'status over 302' => [400, 20, 302, null]; - yield 'status below 301' => [201, 20, 302, null]; - yield 'status 301 with valid expiration' => [301, 20, 301, 'private,max-age=20']; - yield 'status 301 with zero expiration' => [301, 0, 301, 'private,max-age=30']; - yield 'status 301 with negative expiration' => [301, -20, 301, 'private,max-age=30']; - } } diff --git a/module/Core/test/ErrorHandler/NotFoundRedirectHandlerTest.php b/module/Core/test/ErrorHandler/NotFoundRedirectHandlerTest.php index 69cf6c98..83810d22 100644 --- a/module/Core/test/ErrorHandler/NotFoundRedirectHandlerTest.php +++ b/module/Core/test/ErrorHandler/NotFoundRedirectHandlerTest.php @@ -10,13 +10,16 @@ use Laminas\Diactoros\Uri; use Mezzio\Router\Route; use Mezzio\Router\RouteResult; use PHPUnit\Framework\TestCase; +use Prophecy\Argument; use Prophecy\PhpUnit\ProphecyTrait; +use Prophecy\Prophecy\ObjectProphecy; use Psr\Http\Message\ServerRequestInterface; use Psr\Http\Server\MiddlewareInterface; use Psr\Http\Server\RequestHandlerInterface; use Shlinkio\Shlink\Core\Action\RedirectAction; use Shlinkio\Shlink\Core\ErrorHandler\NotFoundRedirectHandler; use Shlinkio\Shlink\Core\Options\NotFoundRedirectOptions; +use Shlinkio\Shlink\Core\Util\RedirectResponseHelperInterface; class NotFoundRedirectHandlerTest extends TestCase { @@ -24,11 +27,13 @@ class NotFoundRedirectHandlerTest extends TestCase private NotFoundRedirectHandler $middleware; private NotFoundRedirectOptions $redirectOptions; + private ObjectProphecy $helper; public function setUp(): void { $this->redirectOptions = new NotFoundRedirectOptions(); - $this->middleware = new NotFoundRedirectHandler($this->redirectOptions, ''); + $this->helper = $this->prophesize(RedirectResponseHelperInterface::class); + $this->middleware = new NotFoundRedirectHandler($this->redirectOptions, $this->helper->reveal(), ''); } /** @@ -43,13 +48,16 @@ class NotFoundRedirectHandlerTest extends TestCase $this->redirectOptions->regular404 = 'regular404'; $this->redirectOptions->baseUrl = 'baseUrl'; + $expectedResp = new Response(); + $buildResp = $this->helper->buildRedirectResponse($expectedRedirectTo)->willReturn($expectedResp); + $next = $this->prophesize(RequestHandlerInterface::class); $handle = $next->handle($request)->willReturn(new Response()); $resp = $this->middleware->process($request, $next->reveal()); - self::assertInstanceOf(Response\RedirectResponse::class, $resp); - self::assertEquals($expectedRedirectTo, $resp->getHeaderLine('Location')); + self::assertSame($expectedResp, $resp); + $buildResp->shouldHaveBeenCalledOnce(); $handle->shouldNotHaveBeenCalled(); } @@ -91,12 +99,15 @@ class NotFoundRedirectHandlerTest extends TestCase $req = ServerRequestFactory::fromGlobals(); $resp = new Response(); + $buildResp = $this->helper->buildRedirectResponse(Argument::cetera()); + $next = $this->prophesize(RequestHandlerInterface::class); $handle = $next->handle($req)->willReturn($resp); $result = $this->middleware->process($req, $next->reveal()); self::assertSame($resp, $result); + $buildResp->shouldNotHaveBeenCalled(); $handle->shouldHaveBeenCalledOnce(); } } diff --git a/module/Core/test/Util/RedirectResponseHelperTest.php b/module/Core/test/Util/RedirectResponseHelperTest.php new file mode 100644 index 00000000..0eb8c0fe --- /dev/null +++ b/module/Core/test/Util/RedirectResponseHelperTest.php @@ -0,0 +1,55 @@ +shortenerOpts = new UrlShortenerOptions(); + $this->helper = new RedirectResponseHelper($this->shortenerOpts); + } + + /** + * @test + * @dataProvider provideRedirectConfigs + */ + public function expectedStatusCodeAndCacheIsReturnedBasedOnConfig( + int $configuredStatus, + int $configuredLifetime, + int $expectedStatus, + ?string $expectedCacheControl + ): void { + $this->shortenerOpts->redirectStatusCode = $configuredStatus; + $this->shortenerOpts->redirectCacheLifetime = $configuredLifetime; + + $response = $this->helper->buildRedirectResponse('destination'); + + self::assertInstanceOf(RedirectResponse::class, $response); + self::assertEquals($expectedStatus, $response->getStatusCode()); + self::assertTrue($response->hasHeader('Location')); + self::assertEquals('destination', $response->getHeaderLine('Location')); + self::assertEquals($expectedCacheControl !== null, $response->hasHeader('Cache-Control')); + self::assertEquals($expectedCacheControl ?? '', $response->getHeaderLine('Cache-Control')); + } + + public function provideRedirectConfigs(): iterable + { + yield 'status 302' => [302, 20, 302, null]; + yield 'status over 302' => [400, 20, 302, null]; + yield 'status below 301' => [201, 20, 302, null]; + yield 'status 301 with valid expiration' => [301, 20, 301, 'private,max-age=20']; + yield 'status 301 with zero expiration' => [301, 0, 301, 'private,max-age=30']; + yield 'status 301 with negative expiration' => [301, -20, 301, 'private,max-age=30']; + } +} From fb022eae686612d1a02f7bc351937f244fc80ebb Mon Sep 17 00:00:00 2001 From: Alejandro Celaya Date: Tue, 10 Nov 2020 18:13:24 +0100 Subject: [PATCH 3/4] #867 Changed use of deprecated functions by their replacements --- module/Core/src/Action/AbstractTrackingAction.php | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/module/Core/src/Action/AbstractTrackingAction.php b/module/Core/src/Action/AbstractTrackingAction.php index 08a35855..b121ae3a 100644 --- a/module/Core/src/Action/AbstractTrackingAction.php +++ b/module/Core/src/Action/AbstractTrackingAction.php @@ -5,6 +5,7 @@ declare(strict_types=1); namespace Shlinkio\Shlink\Core\Action; use Fig\Http\Message\RequestMethodInterface; +use GuzzleHttp\Psr7\Query; use League\Uri\Uri; use Mezzio\Router\Middleware\ImplicitHeadMiddleware; use Psr\Http\Message\ResponseInterface; @@ -23,8 +24,6 @@ use Shlinkio\Shlink\Core\Service\VisitsTrackerInterface; use function array_key_exists; use function array_merge; -use function GuzzleHttp\Psr7\build_query; -use function GuzzleHttp\Psr7\parse_query; abstract class AbstractTrackingAction implements MiddlewareInterface, RequestMethodInterface { @@ -68,13 +67,13 @@ abstract class AbstractTrackingAction implements MiddlewareInterface, RequestMet private function buildUrlToRedirectTo(ShortUrl $shortUrl, array $currentQuery, ?string $disableTrackParam): string { $uri = Uri::createFromString($shortUrl->getLongUrl()); - $hardcodedQuery = parse_query($uri->getQuery() ?? ''); + $hardcodedQuery = Query::parse($uri->getQuery() ?? ''); if ($disableTrackParam !== null) { unset($currentQuery[$disableTrackParam]); } $mergedQuery = array_merge($hardcodedQuery, $currentQuery); - return (string) (empty($mergedQuery) ? $uri : $uri->withQuery(build_query($mergedQuery))); + return (string) (empty($mergedQuery) ? $uri : $uri->withQuery(Query::build($mergedQuery))); } private function shouldTrackRequest(ServerRequestInterface $request, array $query, ?string $disableTrackParam): bool From e99ab66afdafaca518cef6f21cc3383e8eae1af5 Mon Sep 17 00:00:00 2001 From: Alejandro Celaya Date: Tue, 10 Nov 2020 18:33:33 +0100 Subject: [PATCH 4/4] Updated changelog --- CHANGELOG.md | 1 + 1 file changed, 1 insertion(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 3af1a15f..178bbbe3 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -20,6 +20,7 @@ The format is based on [Keep a Changelog](https://keepachangelog.com), and this ### Fixed * [#891](https://github.com/shlinkio/shlink/issues/891) Fixed error when running migrations in postgres due to incorrect return type hint. * [#846](https://github.com/shlinkio/shlink/issues/846) Fixed base image used for the PHP-FPM dev container. +* [#867](https://github.com/shlinkio/shlink/issues/867) Fixed not-found redirects not using proper status (301 or 302) as configured during installation. ## [2.4.0] - 2020-11-08