Merge pull request #1657 from acelaya-forks/feature/extra-method-redirects

Feature/extra method redirects

CHANGELOG.md

@@ -8,6 +8,11 @@ The format is based on [Keep a Changelog](https://keepachangelog.com), and this
 ### Added
 * [#1632](https://github.com/shlinkio/shlink/issues/1632) Added amount of bots, non-bots and total visits to the visits summary endpoint.
 * [#1633](https://github.com/shlinkio/shlink/issues/1633) Added amount of bots, non-bots and total visits to the tag stats endpoint.
+* [#1653](https://github.com/shlinkio/shlink/issues/1653) Added support for all HTTP methods in short URLs, together with two new redirect status codes, 307 and 308.
+
+  Existing Shlink instances will continue to work the same. However, if you decide to set the redirect status codes as 307 or 308, Shlink will also return a redirect for short URLs even when the request method is different from `GET`.
+
+  The status 308 is equivalent to 301, and 307 is equivalent to 302. The difference is that the spec requires the client to respect the original HTTP method when performing the redirect. With 301 and 302, some old clients might perform a `GET` request during the redirect, regardless the original request method.
 
 ### Changed
 * *Nothing*

composer.json

@@ -49,7 +49,7 @@
         "shlinkio/shlink-config": "^2.3",
         "shlinkio/shlink-event-dispatcher": "^2.6",
         "shlinkio/shlink-importer": "^5.0",
-        "shlinkio/shlink-installer": "^8.2",
+        "shlinkio/shlink-installer": "dev-develop#5fcee9b as 8.3",
         "shlinkio/shlink-ip-geolocation": "^3.2",
         "spiral/roadrunner": "^2.11",
         "spiral/roadrunner-jobs": "^2.5",

config/autoload/redirects.global.php

@@ -16,7 +16,7 @@ return [
     ],
 
     'redirects' => [
-        'redirect_status_code' => (int) EnvVars::REDIRECT_STATUS_CODE->loadFromEnv(DEFAULT_REDIRECT_STATUS_CODE),
+        'redirect_status_code' => (int) EnvVars::REDIRECT_STATUS_CODE->loadFromEnv(DEFAULT_REDIRECT_STATUS_CODE->value),
         'redirect_cache_lifetime' => (int) EnvVars::REDIRECT_CACHE_LIFETIME->loadFromEnv(
             DEFAULT_REDIRECT_CACHE_LIFETIME,
         ),

config/config.php

@@ -48,6 +48,7 @@ return (new ConfigAggregator\ConfigAggregator([
     // Routes have to be loaded last
     new ConfigAggregator\PhpFileProvider('config/autoload/routes.config.php'),
 ], 'data/cache/app_config.php', [
-    Core\Config\BasePathPrefixer::class,
-    Core\Config\MultiSegmentSlugProcessor::class,
+    Core\Config\PostProcessor\BasePathPrefixer::class,
+    Core\Config\PostProcessor\MultiSegmentSlugProcessor::class,
+    Core\Config\PostProcessor\ShortUrlMethodsProcessor::class,
 ]))->getMergedConfig();

config/constants.php

@@ -4,12 +4,12 @@ declare(strict_types=1);
 
 namespace Shlinkio\Shlink;
 
-use Fig\Http\Message\StatusCodeInterface;
+use Shlinkio\Shlink\Core\Util\RedirectStatus;
 
 const DEFAULT_DELETE_SHORT_URL_THRESHOLD = 15;
 const DEFAULT_SHORT_CODES_LENGTH = 5;
 const MIN_SHORT_CODES_LENGTH = 4;
-const DEFAULT_REDIRECT_STATUS_CODE = StatusCodeInterface::STATUS_FOUND;
+const DEFAULT_REDIRECT_STATUS_CODE = RedirectStatus::STATUS_302; // Deprecated. Default to 307 for Shlink v4
 const DEFAULT_REDIRECT_CACHE_LIFETIME = 30;
 const LOCAL_LOCK_FACTORY = 'Shlinkio\Shlink\LocalLockFactory';
 const TITLE_TAG_VALUE = '/<title[^>]*>(.*?)<\/title>/i'; // Matches the value inside a html title tag

docker-compose.yml

@@ -102,7 +102,7 @@ services:
 
     shlink_db_mysql:
         container_name: shlink_db_mysql
-        image: mysql:5.7
+        image: mysql:8.0
         ports:
             - "3307:3306"
         volumes:
@@ -175,7 +175,7 @@ services:
 
     shlink_mercure:
         container_name: shlink_mercure
-        image: dunglas/mercure:v0.13
+        image: dunglas/mercure:v0.14
         ports:
             - "3080:80"
         environment:

docs/adr/2023-01-06-support-any-http-method-in-short-urls.md

@@ -0,0 +1,77 @@
+# Support any HTTP method in short URLs
+
+* Status: Accepted
+* Date: 2023-01-06
+
+## Context and problem statement
+
+There has been a report that Shlink behaves as if a short URL was not found when the request HTTP method is not `GET`.
+
+They want it to accept other methods so that they can do things like POSTing stuff that then gets "redirected" to the original URL.
+
+This presents two main problems:
+
+* Changing this could be considered a breaking change, in case someone is relying on this behavior (Shlink to only redirect on `GET`).
+* Shlink currently supports two redirect statuses ([301](https://httpwg.org/specs/rfc9110.html#status.301) and [302](https://httpwg.org/specs/rfc9110.html#status.302)), which can be configured by the server admin.
+
+  For historical reasons, a client might switch from the original method to `GET` when any of these is returned, not resulting in the desired behavior anyway.
+
+  Instead, statuses [308](https://httpwg.org/specs/rfc9110.html#status.308) and [307](https://httpwg.org/specs/rfc9110.html#status.307) should be used.
+
+## Considered options
+
+There's actually two problems to solve here. Some combinations are implicitly required:
+
+* **To support other HTTP methods in short URLs**
+  * Start supporting all HTTP methods.
+  * Introduce a feature flag to allow users decide if they want to support all methods or just `GET`.
+* **To support other redirects statuses (308 and 307)**
+  * Switch to status 308 and 307 and stop using 301 and 302.
+  * Allow users to configure which of the 4 status codes they want to use, insteadof just supporting 301 and 302.
+  * Allow users to configure between two combinations: 301+308 and 302+307, using 301 or 302 for `GET` requests, and 308 or 307 for the rest.
+
+> **Note**
+> I asked on social networks, and these were the results (not too many answers though):
+> * https://fosstodon.org/@shlinkio/109626773392324128
+> * https://twitter.com/shlinkio/status/1610347091741507585
+
+## Decision outcome
+
+Because of backwards compatibility, it feels like the bets option is allowing to configure between 301, 302, 308 and 307.
+
+This has the benefit that we can keep existing behavior intact. Existing instances will continue working only on `GET`, with statuses 301 or 302.
+
+Anyone who wants to opt-in, can switch to 308 or 307, and the short URLs will transparently work on other HTTP methods in that case.
+
+The only drawback is that this difference in the behavior when 308 or 307 are configured needs to be documented, and explained in shlink-installer.
+
+## Pros and Cons of the Options
+
+### Start supporting all HTTP methods
+
+* Good: Because the change in code is pretty simple.
+* Bad: Because it would be potentially a breaking change for anyone trusting current behavior for anything.
+
+### Support HTTP methods via feature flag
+
+* Good: because it would be safer for existing instances and opt-in for anyone interested in this change of behavior.
+* Bad: Because it requires more changes in code.
+* Bad: Because it requires a new config entry in the shlink-installer.
+
+### Switch to statuses 308 and 307
+
+* Good: Because we keep supporting just two status codes.
+* Bad: Because it requires applying mapping/transformation to convert old configurations.
+* Bad: Because it requires changes in shlink-installer.
+
+### Allow users to configure between 301, 302, 308 and 307
+
+* Good: Because it's fully backwards compatible with existing configs.
+* Good: Because it would implicitly allow enabling all HTTP methods if 308 or 307 are selected, and keep only `GET` for 301 and 302, without the need for a separated feature flag.
+* Bad: Because it requires dynamically supporting only `GET` or all methods, depending on the selected status.
+
+### Allow users to configure between 301+308 or 302+307
+
+* Good: Because it would allow a more explicit redirects config, where values are not 301 and 302, but something like "permanent" and "temporary".
+* Bad: Because it implicitly changes the behavior of existing instances, making them respond to redirects with a method other than `GET`, and with a status code other than the one they explicitly configured.
+* Bad: because existing `REDIRECT_STATUS_CODE` env var might not make sense anymore, requiring a new one and logic to map from one to another.

docs/adr/README.md

@@ -2,6 +2,7 @@
 
 Here listed you will find the different architectural decisions taken in the project, including all the reasoning behind it, options considered, and final outcome.
 
+* [2023-01-06 Support any HTTP method in short URLs](2023-01-06-support-any-http-method-in-short-urls.md)
 * [2022-08-05 Support multi-segment custom slugs](2022-08-05-support-multi-segment-custom-slugs.md)
 * [2022-01-15 Update env vars behavior to have precedence over installer options](2022-01-15-update-env-vars-behavior-to-have-precedence-over-installer-options.md)
 * [2021-08-05 Migrate to a new caching library](2021-08-05-migrate-to-a-new-caching-library.md)

module/Core/src/Config/PostProcessor/BasePathPrefixer.php

@@ -2,7 +2,7 @@
 
 declare(strict_types=1);
 
-namespace Shlinkio\Shlink\Core\Config;
+namespace Shlinkio\Shlink\Core\Config\PostProcessor;
 
 use function Functional\map;
 

module/Core/src/Config/PostProcessor/MultiSegmentSlugProcessor.php

@@ -2,7 +2,7 @@
 
 declare(strict_types=1);
 
-namespace Shlinkio\Shlink\Core\Config;
+namespace Shlinkio\Shlink\Core\Config\PostProcessor;
 
 use function Functional\map;
 use function str_replace;

module/Core/src/Config/PostProcessor/ShortUrlMethodsProcessor.php

@@ -0,0 +1,41 @@
+<?php
+
+declare(strict_types=1);
+
+namespace Shlinkio\Shlink\Core\Config\PostProcessor;
+
+use Fig\Http\Message\RequestMethodInterface;
+use Mezzio\Router\Route;
+use Shlinkio\Shlink\Core\Action\RedirectAction;
+use Shlinkio\Shlink\Core\Util\RedirectStatus;
+
+use function array_values;
+use function count;
+use function Functional\partition;
+
+use const Shlinkio\Shlink\DEFAULT_REDIRECT_STATUS_CODE;
+
+class ShortUrlMethodsProcessor
+{
+    public function __invoke(array $config): array
+    {
+        [$redirectRoutes, $rest] = partition(
+            $config['routes'] ?? [],
+            static fn (array $route) => $route['name'] === RedirectAction::class,
+        );
+        if (count($redirectRoutes) === 0) {
+            return $config;
+        }
+
+        [$redirectRoute] = array_values($redirectRoutes);
+        $redirectStatus = RedirectStatus::tryFrom(
+            $config['redirects']['redirect_status_code'] ?? 0,
+        ) ?? DEFAULT_REDIRECT_STATUS_CODE;
+        $redirectRoute['allowed_methods'] = $redirectStatus->isLegacyStatus()
+            ? [RequestMethodInterface::METHOD_GET]
+            : Route::HTTP_METHOD_ANY;
+
+        $config['routes'] = [...$rest, $redirectRoute];
+        return $config;
+    }
+}

module/Core/src/Options/RedirectOptions.php

@@ -4,23 +4,22 @@ declare(strict_types=1);
 
 namespace Shlinkio\Shlink\Core\Options;
 
-use function Functional\contains;
+use Fig\Http\Message\StatusCodeInterface;
+use Shlinkio\Shlink\Core\Util\RedirectStatus;
 
 use const Shlinkio\Shlink\DEFAULT_REDIRECT_CACHE_LIFETIME;
 use const Shlinkio\Shlink\DEFAULT_REDIRECT_STATUS_CODE;
 
 final class RedirectOptions
 {
-    public readonly int $redirectStatusCode;
+    public readonly RedirectStatus $redirectStatusCode;
     public readonly int $redirectCacheLifetime;
 
     public function __construct(
-        int $redirectStatusCode = DEFAULT_REDIRECT_STATUS_CODE,
+        int $redirectStatusCode = StatusCodeInterface::STATUS_FOUND,
         int $redirectCacheLifetime = DEFAULT_REDIRECT_CACHE_LIFETIME,
     ) {
-        $this->redirectStatusCode = contains([301, 302], $redirectStatusCode)
-            ? $redirectStatusCode
-            : DEFAULT_REDIRECT_STATUS_CODE;
+        $this->redirectStatusCode = RedirectStatus::tryFrom($redirectStatusCode) ?? DEFAULT_REDIRECT_STATUS_CODE;
         $this->redirectCacheLifetime = $redirectCacheLifetime > 0
             ? $redirectCacheLifetime
             : DEFAULT_REDIRECT_CACHE_LIFETIME;

module/Core/src/Util/RedirectResponseHelper.php

@@ -4,7 +4,6 @@ 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\RedirectOptions;
@@ -13,17 +12,17 @@ use function sprintf;
 
 class RedirectResponseHelper implements RedirectResponseHelperInterface
 {
-    public function __construct(private RedirectOptions $options)
+    public function __construct(private readonly RedirectOptions $options)
     {
     }
 
     public function buildRedirectResponse(string $location): ResponseInterface
     {
         $statusCode = $this->options->redirectStatusCode;
-        $headers = $statusCode === StatusCodeInterface::STATUS_FOUND ? [] : [
+        $headers = ! $statusCode->allowsCache() ? [] : [
             'Cache-Control' => sprintf('private,max-age=%s', $this->options->redirectCacheLifetime),
         ];
 
-        return new RedirectResponse($location, $statusCode, $headers);
+        return new RedirectResponse($location, $statusCode->value, $headers);
     }
 }

module/Core/src/Util/RedirectStatus.php

@@ -0,0 +1,23 @@
+<?php
+
+namespace Shlinkio\Shlink\Core\Util;
+
+use function Functional\contains;
+
+enum RedirectStatus: int
+{
+    case STATUS_301 = 301; // StatusCodeInterface::STATUS_MOVED_PERMANENTLY;
+    case STATUS_302 = 302; // StatusCodeInterface::STATUS_FOUND;
+    case STATUS_307 = 307; // StatusCodeInterface::STATUS_TEMPORARY_REDIRECT;
+    case STATUS_308 = 308; // StatusCodeInterface::STATUS_PERMANENT_REDIRECT;
+
+    public function allowsCache(): bool
+    {
+        return contains([self::STATUS_301, self::STATUS_308], $this);
+    }
+
+    public function isLegacyStatus(): bool
+    {
+        return contains([self::STATUS_301, self::STATUS_302], $this);
+    }
+}

module/Core/test/Config/PostProcessor/BasePathPrefixerTest.php

@@ -2,10 +2,10 @@
 
 declare(strict_types=1);
 
-namespace ShlinkioTest\Shlink\Core\Config;
+namespace ShlinkioTest\Shlink\Core\Config\PostProcessor;
 
 use PHPUnit\Framework\TestCase;
-use Shlinkio\Shlink\Core\Config\BasePathPrefixer;
+use Shlinkio\Shlink\Core\Config\PostProcessor\BasePathPrefixer;
 
 class BasePathPrefixerTest extends TestCase
 {

module/Core/test/Config/PostProcessor/MultiSegmentSlugProcessorTest.php

@@ -2,10 +2,10 @@
 
 declare(strict_types=1);
 
-namespace ShlinkioTest\Shlink\Core\Config;
+namespace ShlinkioTest\Shlink\Core\Config\PostProcessor;
 
 use PHPUnit\Framework\TestCase;
-use Shlinkio\Shlink\Core\Config\MultiSegmentSlugProcessor;
+use Shlinkio\Shlink\Core\Config\PostProcessor\MultiSegmentSlugProcessor;
 
 class MultiSegmentSlugProcessorTest extends TestCase
 {

module/Core/test/Config/PostProcessor/ShortUrlMethodsProcessorTest.php

@@ -0,0 +1,105 @@
+<?php
+
+declare(strict_types=1);
+
+namespace ShlinkioTest\Shlink\Core\Config\PostProcessor;
+
+use Mezzio\Router\Route;
+use PHPUnit\Framework\TestCase;
+use Shlinkio\Shlink\Core\Action\RedirectAction;
+use Shlinkio\Shlink\Core\Config\PostProcessor\ShortUrlMethodsProcessor;
+
+class ShortUrlMethodsProcessorTest extends TestCase
+{
+    private ShortUrlMethodsProcessor $processor;
+
+    protected function setUp(): void
+    {
+        $this->processor = new ShortUrlMethodsProcessor();
+    }
+
+    /**
+     * @test
+     * @dataProvider provideConfigs
+     */
+    public function onlyFirstRouteIdentifiedAsRedirectIsEditedWithProperAllowedMethods(
+        array $config,
+        ?array $expectedRoutes,
+    ): void {
+        self::assertEquals($expectedRoutes, ($this->processor)($config)['routes'] ?? null);
+    }
+
+    public function provideConfigs(): iterable
+    {
+        $buildConfigWithStatus = static fn (int $status, ?array $expectedAllowedMethods) => [[
+            'routes' => [
+                ['name' => 'foo'],
+                ['name' => 'bar'],
+                ['name' => RedirectAction::class],
+            ],
+            'redirects' => [
+                'redirect_status_code' => $status,
+            ],
+        ], [
+            ['name' => 'foo'],
+            ['name' => 'bar'],
+            [
+                'name' => RedirectAction::class,
+                'allowed_methods' => $expectedAllowedMethods,
+            ],
+        ]];
+
+        yield 'empty config' => [[], null];
+        yield 'empty routes' => [['routes' => []], []];
+        yield 'no redirects route' => [['routes' => $routes = [
+            ['name' => 'foo'],
+            ['name' => 'bar'],
+        ]], $routes];
+        yield 'one redirects route' => [['routes' => [
+            ['name' => 'foo'],
+            ['name' => 'bar'],
+            ['name' => RedirectAction::class],
+        ]], [
+            ['name' => 'foo'],
+            ['name' => 'bar'],
+            [
+                'name' => RedirectAction::class,
+                'allowed_methods' => ['GET'],
+            ],
+        ]];
+        yield 'one redirects route in different location' => [['routes' => [
+            [
+                'name' => RedirectAction::class,
+                'allowed_methods' => ['POST'],
+            ],
+            ['name' => 'foo'],
+            ['name' => 'bar'],
+        ]], [
+            ['name' => 'foo'],
+            ['name' => 'bar'],
+            [
+                'name' => RedirectAction::class,
+                'allowed_methods' => ['GET'],
+            ],
+        ]];
+        yield 'multiple redirects routes' => [['routes' => [
+            ['name' => RedirectAction::class],
+            ['name' => 'foo'],
+            ['name' => 'bar'],
+            ['name' => RedirectAction::class],
+            ['name' => RedirectAction::class],
+        ]], [
+            ['name' => 'foo'],
+            ['name' => 'bar'],
+            [
+                'name' => RedirectAction::class,
+                'allowed_methods' => ['GET'],
+            ],
+        ]];
+        yield 'one redirects route with invalid status code' => $buildConfigWithStatus(500, ['GET']);
+        yield 'one redirects route with 302 status code' => $buildConfigWithStatus(302, ['GET']);
+        yield 'one redirects route with 301 status code' => $buildConfigWithStatus(301, ['GET']);
+        yield 'one redirects route with 307 status code' => $buildConfigWithStatus(307, Route::HTTP_METHOD_ANY);
+        yield 'one redirects route with 308 status code' => $buildConfigWithStatus(308, Route::HTTP_METHOD_ANY);
+    }
+}

module/Core/test/Util/RedirectResponseHelperTest.php

@@ -36,11 +36,15 @@ class RedirectResponseHelperTest extends TestCase
     public function provideRedirectConfigs(): iterable
     {
         yield 'status 302' => [302, 20, 302, null];
-        yield 'status over 302' => [400, 20, 302, null];
+        yield 'status 307' => [307, 20, 307, null];
+        yield 'status over 308' => [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'];
+        yield 'status 308 with valid expiration' => [308, 20, 308, 'private,max-age=20'];
+        yield 'status 308 with zero expiration' => [308, 0, 308, 'private,max-age=30'];
+        yield 'status 308 with negative expiration' => [308, -20, 308, 'private,max-age=30'];
     }
 
     private function helper(?RedirectOptions $options = null): RedirectResponseHelper