Merge pull request #2526 from acelaya-forks/remove-trusted-proxies-workaround

Remove workaround to detect trusted proxies automatically

CHANGELOG.md

@@ -9,7 +9,9 @@ The format is based on [Keep a Changelog](https://keepachangelog.com), and this
 * *Nothing*
 
 ### Changed
-* *Nothing*
+* [#2522](https://github.com/shlinkio/shlink/issues/2522) Shlink no longer tries to detect trusted proxies automatically, when resolving the visitor's IP address, as this is a potential security issue.
+
+    Instead, if you have more than 1 proxy in front of Shlink, you should provide `TRUSTED_PROXIES` env var, with either a comma-separated list of the IP addresses of your proxies, or a number indicating how many proxies are there in front of Shlink.
 
 ### Deprecated
 * *Nothing*

config/autoload/ip-address.global.php

@@ -5,7 +5,6 @@ declare(strict_types=1);
 use RKA\Middleware\IpAddress;
 use RKA\Middleware\Mezzio\IpAddressFactory;
 use Shlinkio\Shlink\Core\Config\EnvVars;
-use Shlinkio\Shlink\Core\Middleware\ReverseForwardedAddressesMiddlewareDecorator;
 
 use function Shlinkio\Shlink\Core\splitByComma;
 
@@ -43,18 +42,6 @@ return (static function (): array {
             'factories' => [
                 IpAddress::class => IpAddressFactory::class,
             ],
-            'delegators' => [
-                // Make middleware decoration transparent to other parts of the code
-                IpAddress::class => [
-                    fn ($c, $n, callable $callback) =>
-                        // If trusted proxies have been provided, use original middleware verbatim, otherwise decorate
-                        // with workaround
-                        $trustedProxies !== null
-                            ? $callback()
-                            : new ReverseForwardedAddressesMiddlewareDecorator($callback()),
-                ],
-            ],
-
         ],
 
     ];

module/Core/src/Middleware/ReverseForwardedAddressesMiddlewareDecorator.php

@@ -1,51 +0,0 @@
-<?php
-
-declare(strict_types=1);
-
-namespace Shlinkio\Shlink\Core\Middleware;
-
-use Psr\Http\Message\ResponseInterface;
-use Psr\Http\Message\ServerRequestInterface;
-use Psr\Http\Server\MiddlewareInterface;
-use Psr\Http\Server\RequestHandlerInterface;
-
-use function array_reverse;
-use function explode;
-use function implode;
-
-/**
- * Decorates a middleware to make sure it gets called with a list of reversed addresses in `X-Forwarded-For`.
- *
- * This is a workaround for a change in behavior introduced in akrabat/ip-address-middleware 2.5, which now
- * takes the first non-trusted-proxy address in that header, starting from the right, instead of the first
- * address starting from the left.
- * That change breaks Shlink's visitor IP resolution when more than one proxy is used, and trusted proxies
- * are not explicitly set for akrabat/ip-address-middleware (which Shlink does not do).
- *
- * A proper solution would require allowing trusted proxies to be configurable, and apply this logic conditionally, only
- * if trusted proxies are not set.
- *
- * @see https://github.com/akrabat/ip-address-middleware/pull/51
- * @deprecated Remove in future major version, and enforce users with multiple reverse proxies to provide the list via
- *             TRUSTED_PROXIES
- */
-readonly class ReverseForwardedAddressesMiddlewareDecorator implements MiddlewareInterface
-{
-    public const string FORWARDED_FOR_HEADER = 'X-Forwarded-For';
-
-    public function __construct(private MiddlewareInterface $wrappedMiddleware)
-    {
-    }
-
-    public function process(ServerRequestInterface $request, RequestHandlerInterface $handler): ResponseInterface
-    {
-        if ($request->hasHeader(self::FORWARDED_FOR_HEADER)) {
-            $request = $request->withHeader(
-                self::FORWARDED_FOR_HEADER,
-                implode(',', array_reverse(explode(',', $request->getHeaderLine(self::FORWARDED_FOR_HEADER)))),
-            );
-        }
-
-        return $this->wrappedMiddleware->process($request, $handler);
-    }
-}

module/Core/test-api/Action/RedirectTest.php

@@ -106,15 +106,6 @@ class RedirectTest extends ApiTestCase
                 'https://example.com/static-ip-address',
             ];
         }
-
-        yield 'rule: IP address in "X-Forwarded-For" together with proxy addresses' => [
-            [
-                RequestOptions::HEADERS => [
-                    'X-Forwarded-For' => '1.2.3.4, 192.168.1.1, 192.168.1.2',
-                ],
-            ],
-            'https://example.com/static-ip-address',
-        ];
     }
 
     /**

module/Core/test/Middleware/ReverseForwardedAddressesMiddlewareDecoratorTest.php

@@ -1,59 +0,0 @@
-<?php
-
-declare(strict_types=1);
-
-namespace ShlinkioTest\Shlink\Core\Middleware;
-
-use Laminas\Diactoros\Response;
-use Laminas\Diactoros\ServerRequestFactory;
-use PHPUnit\Framework\Attributes\Test;
-use PHPUnit\Framework\MockObject\MockObject;
-use PHPUnit\Framework\TestCase;
-use Psr\Http\Message\ServerRequestInterface;
-use Psr\Http\Server\MiddlewareInterface;
-use Psr\Http\Server\RequestHandlerInterface;
-use Shlinkio\Shlink\Core\Middleware\ReverseForwardedAddressesMiddlewareDecorator;
-
-class ReverseForwardedAddressesMiddlewareDecoratorTest extends TestCase
-{
-    private ReverseForwardedAddressesMiddlewareDecorator $middleware;
-    private MockObject & MiddlewareInterface $decoratedMiddleware;
-    private MockObject & RequestHandlerInterface $requestHandler;
-
-    protected function setUp(): void
-    {
-        $this->decoratedMiddleware = $this->createMock(MiddlewareInterface::class);
-        $this->requestHandler = $this->createMock(RequestHandlerInterface::class);
-        $this->middleware = new ReverseForwardedAddressesMiddlewareDecorator($this->decoratedMiddleware);
-    }
-
-    #[Test]
-    public function processesRequestAsIsWhenHeadersIsNotFound(): void
-    {
-        $request = ServerRequestFactory::fromGlobals();
-        $this->decoratedMiddleware->expects($this->once())->method('process')->with(
-            $request,
-            $this->requestHandler,
-        )->willReturn(new Response());
-
-        $this->middleware->process($request, $this->requestHandler);
-    }
-
-    #[Test]
-    public function revertsListOfAddressesWhenHeaderIsFound(): void
-    {
-        $request = ServerRequestFactory::fromGlobals()->withHeader(
-            ReverseForwardedAddressesMiddlewareDecorator::FORWARDED_FOR_HEADER,
-            '1.2.3.4,5.6.7.8,9.10.11.12',
-        );
-
-        $this->decoratedMiddleware->expects($this->once())->method('process')->with(
-            $this->callback(fn (ServerRequestInterface $req): bool => $req->getHeaderLine(
-                ReverseForwardedAddressesMiddlewareDecorator::FORWARDED_FOR_HEADER,
-            ) === '9.10.11.12,5.6.7.8,1.2.3.4'),
-            $this->requestHandler,
-        )->willReturn(new Response());
-
-        $this->middleware->process($request, $this->requestHandler);
-    }
-}