From 9cb316bdfac962f1cf5217162c5bf00a6b3feef4 Mon Sep 17 00:00:00 2001 From: Alejandro Celaya Date: Thu, 1 Aug 2019 18:27:43 +0200 Subject: [PATCH 1/2] Added more headers to inspect while looking for the remote IP address --- config/autoload/ip-address.global.php | 19 +++++++ .../Middleware/IpAddressMiddlewareFactory.php | 4 +- .../IpAddressMiddlewareFactoryTest.php | 54 +++++++++++++++++-- 3 files changed, 73 insertions(+), 4 deletions(-) create mode 100644 config/autoload/ip-address.global.php diff --git a/config/autoload/ip-address.global.php b/config/autoload/ip-address.global.php new file mode 100644 index 00000000..2def62fd --- /dev/null +++ b/config/autoload/ip-address.global.php @@ -0,0 +1,19 @@ + [ + 'headers_to_inspect' => [ + 'Forwarded', + 'X-Forwarded-For', + 'X-Forwarded', + 'X-Cluster-Client-Ip', + 'Client-Ip', + 'X-Real-IP', + 'CF-Connecting-IP', + 'True-Client-IP', + ], + ], + +]; diff --git a/module/Common/src/Middleware/IpAddressMiddlewareFactory.php b/module/Common/src/Middleware/IpAddressMiddlewareFactory.php index b9cd3879..2116d231 100644 --- a/module/Common/src/Middleware/IpAddressMiddlewareFactory.php +++ b/module/Common/src/Middleware/IpAddressMiddlewareFactory.php @@ -23,6 +23,8 @@ class IpAddressMiddlewareFactory implements FactoryInterface */ public function __invoke(ContainerInterface $container, $requestedName, array $options = null): IpAddress { - return new IpAddress(true, [], Visitor::REMOTE_ADDRESS_ATTR); + $config = $container->get('config'); + $headersToInspect = $config['ip_address_resolution']['headers_to_inspect'] ?? []; + return new IpAddress(true, [], Visitor::REMOTE_ADDRESS_ATTR, $headersToInspect); } } diff --git a/module/Common/test/Middleware/IpAddressMiddlewareFactoryTest.php b/module/Common/test/Middleware/IpAddressMiddlewareFactoryTest.php index 41fbafbb..4a3361bd 100644 --- a/module/Common/test/Middleware/IpAddressMiddlewareFactoryTest.php +++ b/module/Common/test/Middleware/IpAddressMiddlewareFactoryTest.php @@ -18,10 +18,15 @@ class IpAddressMiddlewareFactoryTest extends TestCase $this->factory = new IpAddressMiddlewareFactory(); } - /** @test */ - public function returnedInstanceIsProperlyConfigured() + /** + * @test + * @dataProvider provideConfigs + */ + public function returnedInstanceIsProperlyConfigured(array $config, array $expectedHeadersToInspect): void { - $instance = $this->factory->__invoke(new ServiceManager(), ''); + $instance = ($this->factory)(new ServiceManager(['services' => [ + 'config' => $config, + ]]), ''); $ref = new ReflectionObject($instance); $checkProxyHeaders = $ref->getProperty('checkProxyHeaders'); @@ -30,9 +35,52 @@ class IpAddressMiddlewareFactoryTest extends TestCase $trustedProxies->setAccessible(true); $attributeName = $ref->getProperty('attributeName'); $attributeName->setAccessible(true); + $headersToInspect = $ref->getProperty('headersToInspect'); + $headersToInspect->setAccessible(true); $this->assertTrue($checkProxyHeaders->getValue($instance)); $this->assertEquals([], $trustedProxies->getValue($instance)); $this->assertEquals(Visitor::REMOTE_ADDRESS_ATTR, $attributeName->getValue($instance)); + $this->assertEquals($expectedHeadersToInspect, $headersToInspect->getValue($instance)); + } + + public function provideConfigs(): iterable + { + $defaultHeadersToInspect = [ + 'Forwarded', + 'X-Forwarded-For', + 'X-Forwarded', + 'X-Cluster-Client-Ip', + 'Client-Ip', + ]; + + yield 'no ip_address_resolution config' => [[], $defaultHeadersToInspect]; + yield 'no headers_to_inspect config' => [['ip_address_resolution' => []], $defaultHeadersToInspect]; + yield 'null headers_to_inspect' => [['ip_address_resolution' => [ + 'headers_to_inspect' => null, + ]], $defaultHeadersToInspect]; + yield 'empty headers_to_inspect' => [['ip_address_resolution' => [ + 'headers_to_inspect' => [], + ]], $defaultHeadersToInspect]; + yield 'some headers_to_inspect' => [['ip_address_resolution' => [ + 'headers_to_inspect' => [ + 'foo', + 'bar', + 'baz', + ], + ]], [ + 'foo', + 'bar', + 'baz', + ]]; + yield 'some other headers_to_inspect' => [['ip_address_resolution' => [ + 'headers_to_inspect' => [ + 'something', + 'something_else', + ], + ]], [ + 'something', + 'something_else', + ]]; } } From ac08ed7cf9ce3e829263632e4fa9f82eecca647a Mon Sep 17 00:00:00 2001 From: Alejandro Celaya Date: Thu, 1 Aug 2019 18:31:18 +0200 Subject: [PATCH 2/2] Updated changelog --- CHANGELOG.md | 9 +++++++++ config/autoload/ip-address.global.php | 6 +++--- 2 files changed, 12 insertions(+), 3 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 88214c26..1f7cd6f5 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -33,6 +33,14 @@ The format is based on [Keep a Changelog](https://keepachangelog.com), and this Sadly, this feature is not enabled when serving shlink via apache/nginx, where you should still rely on cronjobs. +* [#384](https://github.com/shlinkio/shlink/issues/384) Improved how remote IP addresses are detected. + + This new set of headers is now also inspected looking for the IP address: + + * CF-Connecting-IP + * True-Client-IP + * X-Real-IP + #### Changed * *Nothing* @@ -49,6 +57,7 @@ The format is based on [Keep a Changelog](https://keepachangelog.com), and this * [#416](https://github.com/shlinkio/shlink/issues/416) Fixed error thrown when trying to locate visits after the GeoLite2 DB is downloaded for the first time. * [#424](https://github.com/shlinkio/shlink/issues/424) Update wkhtmltoimage to version 0.12.5 +* [#427](https://github.com/shlinkio/shlink/issues/427) Fixed shlink being unusable after a database error on swoole contexts. ## 1.17.0 - 2019-05-13 diff --git a/config/autoload/ip-address.global.php b/config/autoload/ip-address.global.php index 2def62fd..edbebf69 100644 --- a/config/autoload/ip-address.global.php +++ b/config/autoload/ip-address.global.php @@ -5,14 +5,14 @@ return [ 'ip_address_resolution' => [ 'headers_to_inspect' => [ + 'CF-Connecting-IP', + 'True-Client-IP', + 'X-Real-IP', 'Forwarded', 'X-Forwarded-For', 'X-Forwarded', 'X-Cluster-Client-Ip', 'Client-Ip', - 'X-Real-IP', - 'CF-Connecting-IP', - 'True-Client-IP', ], ],