Workaround for IP resolution from x-Forwarded-For with multiple proxies

CHANGELOG.md

@@ -4,6 +4,28 @@ All notable changes to this project will be documented in this file.
 
 The format is based on [Keep a Changelog](https://keepachangelog.com), and this project adheres to [Semantic Versioning](https://semver.org).
 
+## [Unreleased]
+### Added
+* *Nothing*
+
+### Changed
+* *Nothing*
+
+### Deprecated
+* *Nothing*
+
+### Removed
+* *Nothing*
+
+### Fixed
+* [#2351](https://github.com/shlinkio/shlink/issues/2351) Fix visitor IP address resolution when Shlink is served behind more than one reverse proxy.
+
+  This regression was introduced due to a change in behavior in `akrabat/rka-ip-address-middleware`, that now picks the first address from the right after excluding all trusted proxies.
+
+  Since Shlink does not set trusted proxies, this means the first IP from the right is now picked instead of the first from the left, so we now reverse the list before trying to resolve the IP.
+
+  In the future, Shlink will allow you to define trusted proxies, to avoid other potential side effects because of this reversing of the list.
+
 ## [4.4.2] - 2025-01-29
 ### Added
 * *Nothing*

composer.json

@@ -18,7 +18,7 @@
         "ext-json": "*",
         "ext-mbstring": "*",
         "ext-pdo": "*",
-        "akrabat/ip-address-middleware": "^2.5",
+        "akrabat/ip-address-middleware": "^2.6",
         "cakephp/chronos": "^3.1",
         "doctrine/dbal": "^4.2",
         "doctrine/migrations": "^3.8",

config/autoload/ip-address.global.php

@@ -2,8 +2,10 @@
 
 declare(strict_types=1);
 
+use Laminas\ServiceManager\AbstractFactory\ConfigAbstractFactory;
 use RKA\Middleware\IpAddress;
 use RKA\Middleware\Mezzio\IpAddressFactory;
+use Shlinkio\Shlink\Core\Middleware\ReverseForwardedAddressesMiddlewareDecorator;
 
 use const Shlinkio\Shlink\IP_ADDRESS_REQUEST_ATTRIBUTE;
 
@@ -30,8 +32,19 @@ return [
 
     'dependencies' => [
         'factories' => [
-            IpAddress::class => IpAddressFactory::class,
+//            IpAddress::class => IpAddressFactory::class,
+            'actual_ip_address_middleware' => IpAddressFactory::class,
+            ReverseForwardedAddressesMiddlewareDecorator::class => ConfigAbstractFactory::class,
+        ],
+        'aliases' => [
+            // Make sure the decorated middleware is resolved when getting IpAddress::class, to make this decoration
+            // transparent for other parts of the code
+            IpAddress::class => ReverseForwardedAddressesMiddlewareDecorator::class,
         ],
     ],
 
+    ConfigAbstractFactory::class => [
+        ReverseForwardedAddressesMiddlewareDecorator::class => ['actual_ip_address_middleware'],
+    ],
+
 ];

module/Core/src/Middleware/ReverseForwardedAddressesMiddlewareDecorator.php

@@ -0,0 +1,49 @@
+<?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
+ */
+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

@@ -90,7 +90,8 @@ class RedirectTest extends ApiTestCase
         ];
 
         $ipAddressConfig = require __DIR__ . '/../../../../config/autoload/ip-address.global.php';
-        foreach ($ipAddressConfig['rka']['ip_address']['headers_to_inspect'] as $header) {
+        $headers = $ipAddressConfig['rka']['ip_address']['headers_to_inspect'];
+        foreach ($headers as $header) {
             yield sprintf('rule: IP address in "%s" header', $header) => [
                 [
                     RequestOptions::HEADERS => [$header => $header !== 'Forwarded' ? '1.2.3.4' : 'for=1.2.3.4'],
@@ -98,6 +99,15 @@ 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

@@ -0,0 +1,59 @@
+<?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);
+    }
+}