Merge pull request #893 from acelaya-forks/feature/wrong-redirect-status

Feature/wrong redirect status
shlinkio · Nov 10, 2020 · 912f287 · 912f287
2 parents 4dbcf68 + e99ab66
commit 912f287
Show file tree

Hide file tree

Showing 10 changed files with 159 additions and 72 deletions.
diff --git 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

diff --git a/module/Core/config/dependencies.config.php b/module/Core/config/dependencies.config.php
@@ -36,6 +36,7 @@
 
             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 @@
     ],
 
     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 @@
 
         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/AbstractTrackingAction.php b/module/Core/src/Action/AbstractTrackingAction.php
@@ -5,6 +5,7 @@
 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 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 @@ public function process(ServerRequestInterface $request, RequestHandlerInterface
     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

diff --git a/module/Core/src/Action/RedirectAction.php b/module/Core/src/Action/RedirectAction.php
@@ -5,40 +5,33 @@
 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;
 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
@@ -4,27 +4,32 @@
 
 namespace Shlinkio\Shlink\Core\ErrorHandler;
 
-use Laminas\Diactoros\Response;
 use Mezzio\Router\RouteResult;
 use Psr\Http\Message\ResponseInterface;
 use Psr\Http\Message\ServerRequestInterface;
 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 Shlinkio\Shlink\Core\Util\RedirectResponseHelperInterface;
 
 use function rtrim;
 
 class NotFoundRedirectHandler implements MiddlewareInterface
 {
-    private NotFoundRedirectOptions $redirectOptions;
+    private Options\NotFoundRedirectOptions $redirectOptions;
+    private RedirectResponseHelperInterface $redirectResponseHelper;
     private string $shlinkBasePath;
 
-    public function __construct(NotFoundRedirectOptions $redirectOptions, 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
@@ -41,19 +46,23 @@ private function createRedirectResponse(RouteResult $routeResult, UriInterface $
         $isBaseUrl = rtrim($uri->getPath(), '/') === $this->shlinkBasePath;
 
         if ($isBaseUrl && $this->redirectOptions->hasBaseUrlRedirect()) {
-            return new Response\RedirectResponse($this->redirectOptions->getBaseUrlRedirect());
+            return $this->redirectResponseHelper->buildRedirectResponse($this->redirectOptions->getBaseUrlRedirect());
         }
 
         if (!$isBaseUrl && $routeResult->isFailure() && $this->redirectOptions->hasRegular404Redirect()) {
-            return new Response\RedirectResponse($this->redirectOptions->getRegular404Redirect());
+            return $this->redirectResponseHelper->buildRedirectResponse(
+                $this->redirectOptions->getRegular404Redirect(),
+            );
         }
 
         if (
             $routeResult->isSuccess() &&
             $routeResult->getMatchedRouteName() === RedirectAction::class &&
             $this->redirectOptions->hasInvalidShortUrlRedirect()
         ) {
-            return new Response\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
@@ -0,0 +1,32 @@
+<?php
+
+declare(strict_types=1);
+
+namespace Shlinkio\Shlink\Core\Util;
+
+use Fig\Http\Message\StatusCodeInterface;
+use Laminas\Diactoros\Response\RedirectResponse;
+use Psr\Http\Message\ResponseInterface;
+use Shlinkio\Shlink\Core\Options\UrlShortenerOptions;
+
+use function sprintf;
+
+class RedirectResponseHelper implements RedirectResponseHelperInterface
+{
+    private UrlShortenerOptions $options;
+
+    public function __construct(UrlShortenerOptions $options)
+    {
+        $this->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
@@ -0,0 +1,12 @@
+<?php
+
+declare(strict_types=1);
+
+namespace Shlinkio\Shlink\Core\Util;
+
+use Psr\Http\Message\ResponseInterface;
+
+interface RedirectResponseHelperInterface
+{
+    public function buildRedirectResponse(string $location): ResponseInterface;
+}
diff --git a/module/Core/test/Action/RedirectActionTest.php b/module/Core/test/Action/RedirectActionTest.php
@@ -20,6 +20,7 @@
 use Shlinkio\Shlink\Core\Options;
 use Shlinkio\Shlink\Core\Service\ShortUrl\ShortUrlResolverInterface;
 use Shlinkio\Shlink\Core\Service\VisitsTrackerInterface;
+use Shlinkio\Shlink\Core\Util\RedirectResponseHelperInterface;
 
 use function array_key_exists;
 
@@ -30,19 +31,19 @@ class RedirectActionTest extends TestCase
     private RedirectAction $action;
     private ObjectProphecy $urlResolver;
     private ObjectProphecy $visitTracker;
-    private Options\UrlShortenerOptions $shortenerOpts;
+    private ObjectProphecy $redirectRespHelper;
 
     public function setUp(): void
     {
         $this->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 @@ public function redirectionIsPerformedToLongUrl(string $expectedUrl, array $quer
         )->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 @@ public function trackingIsDisabledWhenRequestIsForwardedFromHead(): void
         $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 @@ public function trackingIsDisabledWhenRequestIsForwardedFromHead(): void
                                         );
         $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'];
-    }
 }