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

Feature/wrong redirect status

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

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 => [

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

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 Shlinkio\Shlink\Core\Util\RedirectResponseHelperInterface;
-use function sprintf;
 
 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();
+        return $this->redirectResponseHelper->buildRedirectResponse($longUrl);
-        $headers = $statusCode === self::STATUS_FOUND ? [] : [
-            'Cache-Control' => sprintf('private,max-age=%s', $this->urlShortenerOptions->redirectCacheLifetime()),
-        ];
-
-        return new RedirectResponse($longUrl, $statusCode, $headers);
     }
 
     protected function createErrorResp(ServerRequestInterface $request, RequestHandlerInterface $handler): Response

module/Core/src/ErrorHandler/NotFoundRedirectHandler.php

@@ -4,7 +4,6 @@ declare(strict_types=1);
 
 namespace Shlinkio\Shlink\Core\ErrorHandler;
 
-use Laminas\Diactoros\Response;
 use Mezzio\Router\RouteResult;
 use Psr\Http\Message\ResponseInterface;
 use Psr\Http\Message\ServerRequestInterface;
@@ -12,19 +11,25 @@ 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,11 +46,13 @@ class NotFoundRedirectHandler implements MiddlewareInterface
         $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 (
@@ -53,7 +60,9 @@ class NotFoundRedirectHandler implements MiddlewareInterface
             $routeResult->getMatchedRouteName() === RedirectAction::class &&
             $this->redirectOptions->hasInvalidShortUrlRedirect()
         ) {
-            return new Response\RedirectResponse($this->redirectOptions->getInvalidShortUrlRedirect());
+            return $this->redirectResponseHelper->buildRedirectResponse(
+                $this->redirectOptions->getInvalidShortUrlRedirect(),
+            );
         }
 
         return null;

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);
+    }
+}

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;
+}

module/Core/test/Action/RedirectActionTest.php

@@ -20,6 +20,7 @@ use Shlinkio\Shlink\Core\Model\ShortUrlIdentifier;
 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 @@ 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::assertSame($expectedResp, $response);
-        self::assertEquals(302, $response->getStatusCode());
+        $buildResp->shouldHaveBeenCalledOnce();
-        self::assertTrue($response->hasHeader('Location'));
-        self::assertEquals($expectedUrl, $response->getHeaderLine('Location'));
         $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'];
-    }
 }

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::assertSame($expectedResp, $resp);
-        self::assertEquals($expectedRedirectTo, $resp->getHeaderLine('Location'));
+        $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();
     }
 }

module/Core/test/Util/RedirectResponseHelperTest.php

@@ -0,0 +1,55 @@
+<?php
+
+declare(strict_types=1);
+
+namespace ShlinkioTest\Shlink\Core\Util;
+
+use Laminas\Diactoros\Response\RedirectResponse;
+use PHPUnit\Framework\TestCase;
+use Shlinkio\Shlink\Core\Options\UrlShortenerOptions;
+use Shlinkio\Shlink\Core\Util\RedirectResponseHelper;
+
+class RedirectResponseHelperTest extends TestCase
+{
+    private RedirectResponseHelper $helper;
+    private UrlShortenerOptions $shortenerOpts;
+
+    protected function setUp(): void
+    {
+        $this->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'];
+    }
+}