diff --git a/CHANGELOG.md b/CHANGELOG.md index e82ef13b..e05271c4 100644 --- a/CHANGELOG.md +++ b/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* diff --git a/composer.json b/composer.json index 02204c8b..c66a5f73 100644 --- a/composer.json +++ b/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", diff --git a/config/autoload/ip-address.global.php b/config/autoload/ip-address.global.php index 9d531040..78f5bc6d 100644 --- a/config/autoload/ip-address.global.php +++ b/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'], + ], + ]; diff --git a/module/Core/src/Middleware/ReverseForwardedAddressesMiddlewareDecorator.php b/module/Core/src/Middleware/ReverseForwardedAddressesMiddlewareDecorator.php new file mode 100644 index 00000000..71d7545c --- /dev/null +++ b/module/Core/src/Middleware/ReverseForwardedAddressesMiddlewareDecorator.php @@ -0,0 +1,49 @@ +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); + } +} diff --git a/module/Core/test-api/Action/RedirectTest.php b/module/Core/test-api/Action/RedirectTest.php index 2cfe9417..799851c7 100644 --- a/module/Core/test-api/Action/RedirectTest.php +++ b/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', + ]; } /** diff --git a/module/Core/test/Middleware/ReverseForwardedAddressesMiddlewareDecoratorTest.php b/module/Core/test/Middleware/ReverseForwardedAddressesMiddlewareDecoratorTest.php new file mode 100644 index 00000000..d3e1bd5e --- /dev/null +++ b/module/Core/test/Middleware/ReverseForwardedAddressesMiddlewareDecoratorTest.php @@ -0,0 +1,59 @@ +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); + } +}