From 5be7f839f3dfc106a208bc8fd7e116b3a534bcd3 Mon Sep 17 00:00:00 2001 From: Alejandro Celaya Date: Thu, 18 Oct 2018 19:19:42 +0200 Subject: [PATCH 1/9] Ensured visits with empty remote address are not tried to be located --- .../Command/Visit/ProcessVisitsCommand.php | 8 ++++++ .../Visit/ProcessVisitsCommandTest.php | 28 +++++++++++-------- module/Core/src/Entity/Visit.php | 5 ++++ 3 files changed, 30 insertions(+), 11 deletions(-) diff --git a/module/CLI/src/Command/Visit/ProcessVisitsCommand.php b/module/CLI/src/Command/Visit/ProcessVisitsCommand.php index 2a6980c9..871e513d 100644 --- a/module/CLI/src/Command/Visit/ProcessVisitsCommand.php +++ b/module/CLI/src/Command/Visit/ProcessVisitsCommand.php @@ -59,6 +59,14 @@ class ProcessVisitsCommand extends Command $count = 0; foreach ($visits as $visit) { + if (! $visit->hasRemoteAddr()) { + $io->writeln( + sprintf('%s', $this->translator->translate('Ignored visit with no IP address')), + OutputInterface::VERBOSITY_VERBOSE + ); + continue; + } + $ipAddr = $visit->getRemoteAddr(); $io->write(sprintf('%s %s', $this->translator->translate('Processing IP'), $ipAddr)); if ($ipAddr === IpAddress::LOCALHOST) { diff --git a/module/CLI/test/Command/Visit/ProcessVisitsCommandTest.php b/module/CLI/test/Command/Visit/ProcessVisitsCommandTest.php index 174ce3fb..3b91c013 100644 --- a/module/CLI/test/Command/Visit/ProcessVisitsCommandTest.php +++ b/module/CLI/test/Command/Visit/ProcessVisitsCommandTest.php @@ -11,8 +11,11 @@ use Shlinkio\Shlink\Common\Service\IpApiLocationResolver; use Shlinkio\Shlink\Core\Entity\Visit; use Shlinkio\Shlink\Core\Service\VisitService; use Symfony\Component\Console\Application; +use Symfony\Component\Console\Output\OutputInterface; use Symfony\Component\Console\Tester\CommandTester; use Zend\I18n\Translator\Translator; +use function count; +use function round; class ProcessVisitsCommandTest extends TestCase { @@ -67,15 +70,15 @@ class ProcessVisitsCommandTest extends TestCase 'command' => 'visit:process', ]); $output = $this->commandTester->getDisplay(); - $this->assertEquals(0, \strpos($output, 'Processing IP 1.2.3.0')); - $this->assertGreaterThan(0, \strpos($output, 'Processing IP 4.3.2.0')); - $this->assertGreaterThan(0, \strpos($output, 'Processing IP 12.34.56.0')); + $this->assertContains('Processing IP 1.2.3.0', $output); + $this->assertContains('Processing IP 4.3.2.0', $output); + $this->assertContains('Processing IP 12.34.56.0', $output); } /** * @test */ - public function localhostAddressIsIgnored() + public function localhostAndEmptyAddressIsIgnored() { $visits = [ (new Visit())->setRemoteAddr('1.2.3.4'), @@ -83,19 +86,22 @@ class ProcessVisitsCommandTest extends TestCase (new Visit())->setRemoteAddr('12.34.56.78'), (new Visit())->setRemoteAddr('127.0.0.1'), (new Visit())->setRemoteAddr('127.0.0.1'), + (new Visit())->setRemoteAddr(''), + (new Visit())->setRemoteAddr(null), ]; $this->visitService->getUnlocatedVisits()->willReturn($visits) ->shouldBeCalledTimes(1); - $this->visitService->saveVisit(Argument::any())->shouldBeCalledTimes(\count($visits) - 2); + $this->visitService->saveVisit(Argument::any())->shouldBeCalledTimes(count($visits) - 4); $this->ipResolver->resolveIpLocation(Argument::any())->willReturn([]) - ->shouldBeCalledTimes(\count($visits) - 2); + ->shouldBeCalledTimes(count($visits) - 4); $this->commandTester->execute([ 'command' => 'visit:process', - ]); + ], ['verbosity' => OutputInterface::VERBOSITY_VERBOSE]); $output = $this->commandTester->getDisplay(); - $this->assertGreaterThan(0, \strpos($output, 'Ignored localhost address')); + $this->assertContains('Ignored localhost address', $output); + $this->assertContains('Ignored visit with no IP address', $output); } /** @@ -130,8 +136,8 @@ class ProcessVisitsCommandTest extends TestCase 'command' => 'visit:process', ]); - $getApiLimit->shouldHaveBeenCalledTimes(\count($visits)); - $getApiInterval->shouldHaveBeenCalledTimes(\round(\count($visits) / $apiLimit)); - $resolveIpLocation->shouldHaveBeenCalledTimes(\count($visits)); + $getApiLimit->shouldHaveBeenCalledTimes(count($visits)); + $getApiInterval->shouldHaveBeenCalledTimes(round(count($visits) / $apiLimit)); + $resolveIpLocation->shouldHaveBeenCalledTimes(count($visits)); } } diff --git a/module/Core/src/Entity/Visit.php b/module/Core/src/Entity/Visit.php index 81b01ba4..bad18bfe 100644 --- a/module/Core/src/Entity/Visit.php +++ b/module/Core/src/Entity/Visit.php @@ -102,6 +102,11 @@ class Visit extends AbstractEntity implements \JsonSerializable return $this; } + public function hasRemoteAddr(): bool + { + return ! empty($this->remoteAddr); + } + private function obfuscateAddress(?string $address): ?string { // Localhost addresses do not need to be obfuscated From 57714b373c62a1841e14bd80ee4838ee54d1b84e Mon Sep 17 00:00:00 2001 From: Alejandro Celaya Date: Thu, 18 Oct 2018 19:23:07 +0200 Subject: [PATCH 2/9] Added php 7.3 to the travis build matrix --- .travis.yml | 1 + 1 file changed, 1 insertion(+) diff --git a/.travis.yml b/.travis.yml index e290684f..27264cbd 100644 --- a/.travis.yml +++ b/.travis.yml @@ -9,6 +9,7 @@ branches: php: - 7.1 - 7.2 + - 7.3 before_install: - phpenv config-add data/infra/travis-php/memcached.ini From db1304c11a274093055144983ec32f55212a0ae4 Mon Sep 17 00:00:00 2001 From: Alejandro Celaya Date: Thu, 18 Oct 2018 19:24:02 +0200 Subject: [PATCH 3/9] Added unreleased changes to changelog --- CHANGELOG.md | 28 ++++++++++++++++++++++++++++ 1 file changed, 28 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 486dddac..2da4bc21 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,5 +1,33 @@ # CHANGELOG +All notable changes to this project will be documented in this file. + +The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/), and this project adheres to [Semantic Versioning](https://semver.org). + + +## [Unreleased] + +#### Added + +* [#233](https://github.com/shlinkio/shlink/issues/233) Added support for PHP 7.3 + +#### Changed + +* *Nothing* + +#### Deprecated + +* *Nothing* + +#### Removed + +* *Nothing* + +#### Fixed + +* *Nothing* + + ## 1.13.1 - 2018-10-16 #### Added From c25b5f99388767c98e0cef6fe80fbc00f8c5a26e Mon Sep 17 00:00:00 2001 From: Alejandro Celaya Date: Thu, 18 Oct 2018 19:35:30 +0200 Subject: [PATCH 4/9] Allowed failures on PHP 7.3 until a fix is found --- .travis.yml | 4 ++++ CHANGELOG.md | 2 +- 2 files changed, 5 insertions(+), 1 deletion(-) diff --git a/.travis.yml b/.travis.yml index 27264cbd..76428e67 100644 --- a/.travis.yml +++ b/.travis.yml @@ -11,6 +11,10 @@ php: - 7.2 - 7.3 +matrix: + allow_failures: + - php: 7.3 + before_install: - phpenv config-add data/infra/travis-php/memcached.ini - phpenv config-add data/infra/travis-php/apcu.ini diff --git a/CHANGELOG.md b/CHANGELOG.md index 2da4bc21..b7f237a8 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -9,7 +9,7 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/), #### Added -* [#233](https://github.com/shlinkio/shlink/issues/233) Added support for PHP 7.3 +* [#233](https://github.com/shlinkio/shlink/issues/233) Added PHP 7.3 to build matrix allowing its failure. #### Changed From 99f45d885327b4705a04677da6554caf8ea30991 Mon Sep 17 00:00:00 2001 From: Alejandro Celaya Date: Thu, 18 Oct 2018 19:53:50 +0200 Subject: [PATCH 5/9] Installed and registered new middleware to process IP addresses from request --- composer.json | 1 + module/Common/config/dependencies.config.php | 26 +++++++------- .../Middleware/IpAddressMiddlewareFactory.php | 27 ++++++++++++++ .../IpAddressMiddlewareFactoryTest.php | 36 +++++++++++++++++++ 4 files changed, 77 insertions(+), 13 deletions(-) create mode 100644 module/Common/src/Middleware/IpAddressMiddlewareFactory.php create mode 100644 module/Common/test/Middleware/IpAddressMiddlewareFactoryTest.php diff --git a/composer.json b/composer.json index 1bb4ac25..c1fd4d5c 100644 --- a/composer.json +++ b/composer.json @@ -16,6 +16,7 @@ "ext-json": "*", "ext-pdo": "*", "acelaya/ze-content-based-error-handler": "^2.2", + "akrabat/ip-address-middleware": "^1.0", "cakephp/chronos": "^1.2", "cocur/slugify": "^3.0", "doctrine/cache": "^1.6", diff --git a/module/Common/config/dependencies.config.php b/module/Common/config/dependencies.config.php index e9422d38..6278d63b 100644 --- a/module/Common/config/dependencies.config.php +++ b/module/Common/config/dependencies.config.php @@ -1,16 +1,14 @@ [ 'factories' => [ EntityManager::class => Factory\EntityManagerFactory::class, - GuzzleHttp\Client::class => InvokableFactory::class, + GuzzleClient::class => InvokableFactory::class, Cache::class => Factory\CacheFactory::class, 'Logger_Shlink' => Factory\LoggerFactory::class, Filesystem::class => InvokableFactory::class, Translator::class => Factory\TranslatorFactory::class, - TranslatorExtension::class => ConfigAbstractFactory::class, - LocaleMiddleware::class => ConfigAbstractFactory::class, + Template\Extension\TranslatorExtension::class => ConfigAbstractFactory::class, + + Middleware\LocaleMiddleware::class => ConfigAbstractFactory::class, + IpAddress::class => Middleware\IpAddressMiddlewareFactory::class, Image\ImageBuilder::class => Image\ImageBuilderFactory::class, @@ -37,7 +37,7 @@ return [ ], 'aliases' => [ 'em' => EntityManager::class, - 'httpClient' => GuzzleHttp\Client::class, + 'httpClient' => GuzzleClient::class, 'translator' => Translator::class, 'logger' => LoggerInterface::class, Logger::class => 'Logger_Shlink', @@ -49,11 +49,11 @@ return [ ], ConfigAbstractFactory::class => [ - TranslatorExtension::class => ['translator'], - LocaleMiddleware::class => ['translator'], + Template\Extension\TranslatorExtension::class => ['translator'], + Middleware\LocaleMiddleware::class => ['translator'], Service\IpApiLocationResolver::class => ['httpClient'], Service\PreviewGenerator::class => [ - ImageBuilder::class, + Image\ImageBuilder::class, Filesystem::class, 'config.preview_generation.files_location', ], diff --git a/module/Common/src/Middleware/IpAddressMiddlewareFactory.php b/module/Common/src/Middleware/IpAddressMiddlewareFactory.php new file mode 100644 index 00000000..5d988a2e --- /dev/null +++ b/module/Common/src/Middleware/IpAddressMiddlewareFactory.php @@ -0,0 +1,27 @@ +factory = new IpAddressMiddlewareFactory(); + } + + /** + * @test + */ + public function returnedInstanceIsProperlyConfigured() + { + $instance = $this->factory->__invoke(new ServiceManager(), ''); + + $ref = new ReflectionObject($instance); + $checkProxyHeaders = $ref->getProperty('checkProxyHeaders'); + $checkProxyHeaders->setAccessible(true); + $trustedProxies = $ref->getProperty('trustedProxies'); + $trustedProxies->setAccessible(true); + + $this->assertTrue($checkProxyHeaders->getValue($instance)); + $this->assertEquals([], $trustedProxies->getValue($instance)); + } +} From 545094cddf76969dba7a1c3fda7d2f3021e923f1 Mon Sep 17 00:00:00 2001 From: Alejandro Celaya Date: Thu, 18 Oct 2018 20:19:22 +0200 Subject: [PATCH 6/9] Used middleware from library to actually find visitor IP addresses --- .../Middleware/IpAddressMiddlewareFactory.php | 4 +- module/Common/src/Util/IpAddress.php | 12 +++-- .../IpAddressMiddlewareFactoryTest.php | 3 ++ module/Core/config/routes.config.php | 11 ++++- .../src/Action/AbstractTrackingAction.php | 8 +++- module/Core/src/Model/Visitor.php | 47 +++++++++++++++++++ module/Core/src/Service/VisitsTracker.php | 32 +++---------- .../src/Service/VisitsTrackerInterface.php | 9 ++-- .../Core/test/Service/VisitsTrackerTest.php | 9 ++-- 9 files changed, 90 insertions(+), 45 deletions(-) create mode 100644 module/Core/src/Model/Visitor.php diff --git a/module/Common/src/Middleware/IpAddressMiddlewareFactory.php b/module/Common/src/Middleware/IpAddressMiddlewareFactory.php index 5d988a2e..a8689e55 100644 --- a/module/Common/src/Middleware/IpAddressMiddlewareFactory.php +++ b/module/Common/src/Middleware/IpAddressMiddlewareFactory.php @@ -11,6 +11,8 @@ use Zend\ServiceManager\Factory\FactoryInterface; class IpAddressMiddlewareFactory implements FactoryInterface { + public const REMOTE_ADDRESS = 'remote_address'; + /** * Create an object * @@ -22,6 +24,6 @@ class IpAddressMiddlewareFactory implements FactoryInterface */ public function __invoke(ContainerInterface $container, $requestedName, array $options = null): IpAddress { - return new IpAddress(true, []); + return new IpAddress(true, [], self::REMOTE_ADDRESS); } } diff --git a/module/Common/src/Util/IpAddress.php b/module/Common/src/Util/IpAddress.php index efa6ba9e..5e391294 100644 --- a/module/Common/src/Util/IpAddress.php +++ b/module/Common/src/Util/IpAddress.php @@ -4,6 +4,10 @@ declare(strict_types=1); namespace Shlinkio\Shlink\Common\Util; use Shlinkio\Shlink\Common\Exception\WrongIpException; +use function count; +use function explode; +use function implode; +use function trim; final class IpAddress { @@ -43,9 +47,9 @@ final class IpAddress */ public static function fromString(string $address): self { - $address = \trim($address); - $parts = \explode('.', $address); - if (\count($parts) !== self::IPV4_PARTS_COUNT) { + $address = trim($address); + $parts = explode('.', $address); + if (count($parts) !== self::IPV4_PARTS_COUNT) { throw WrongIpException::fromIpAddress($address); } @@ -64,7 +68,7 @@ final class IpAddress public function __toString(): string { - return \implode('.', [ + return implode('.', [ $this->firstOctet, $this->secondOctet, $this->thirdOctet, diff --git a/module/Common/test/Middleware/IpAddressMiddlewareFactoryTest.php b/module/Common/test/Middleware/IpAddressMiddlewareFactoryTest.php index ab6dc95c..2087e5c3 100644 --- a/module/Common/test/Middleware/IpAddressMiddlewareFactoryTest.php +++ b/module/Common/test/Middleware/IpAddressMiddlewareFactoryTest.php @@ -29,8 +29,11 @@ class IpAddressMiddlewareFactoryTest extends TestCase $checkProxyHeaders->setAccessible(true); $trustedProxies = $ref->getProperty('trustedProxies'); $trustedProxies->setAccessible(true); + $attributeName = $ref->getProperty('attributeName'); + $attributeName->setAccessible(true); $this->assertTrue($checkProxyHeaders->getValue($instance)); $this->assertEquals([], $trustedProxies->getValue($instance)); + $this->assertEquals(IpAddressMiddlewareFactory::REMOTE_ADDRESS, $attributeName->getValue($instance)); } } diff --git a/module/Core/config/routes.config.php b/module/Core/config/routes.config.php index 2d8c80b3..a7aabd5b 100644 --- a/module/Core/config/routes.config.php +++ b/module/Core/config/routes.config.php @@ -1,6 +1,7 @@ 'long-url-redirect', 'path' => '/{shortCode}', - 'middleware' => Action\RedirectAction::class, + 'middleware' => [ + IpAddress::class, + Action\RedirectAction::class, + ], 'allowed_methods' => ['GET'], ], [ 'name' => 'pixel-tracking', 'path' => '/{shortCode}/track', - 'middleware' => Action\PixelAction::class, + 'middleware' => [ + IpAddress::class, + Action\PixelAction::class, + ], 'allowed_methods' => ['GET'], ], [ diff --git a/module/Core/src/Action/AbstractTrackingAction.php b/module/Core/src/Action/AbstractTrackingAction.php index cfb4223c..778396d9 100644 --- a/module/Core/src/Action/AbstractTrackingAction.php +++ b/module/Core/src/Action/AbstractTrackingAction.php @@ -9,9 +9,11 @@ use Psr\Http\Server\MiddlewareInterface; use Psr\Http\Server\RequestHandlerInterface; use Psr\Log\LoggerInterface; use Psr\Log\NullLogger; +use Shlinkio\Shlink\Common\Middleware\IpAddressMiddlewareFactory; use Shlinkio\Shlink\Core\Action\Util\ErrorResponseBuilderTrait; use Shlinkio\Shlink\Core\Exception\EntityDoesNotExistException; use Shlinkio\Shlink\Core\Exception\InvalidShortCodeException; +use Shlinkio\Shlink\Core\Model\Visitor; use Shlinkio\Shlink\Core\Options\AppOptions; use Shlinkio\Shlink\Core\Service\UrlShortenerInterface; use Shlinkio\Shlink\Core\Service\VisitsTrackerInterface; @@ -69,7 +71,11 @@ abstract class AbstractTrackingAction implements MiddlewareInterface // Track visit to this short code if ($disableTrackParam === null || ! \array_key_exists($disableTrackParam, $query)) { - $this->visitTracker->track($shortCode, $request); + $this->visitTracker->track($shortCode, new Visitor( + $request->getHeaderLine('User-Agent'), + $request->getHeaderLine('Referer'), + $request->getAttribute(IpAddressMiddlewareFactory::REMOTE_ADDRESS) + )); } return $this->createResp($url->getLongUrl()); diff --git a/module/Core/src/Model/Visitor.php b/module/Core/src/Model/Visitor.php new file mode 100644 index 00000000..c6a828f0 --- /dev/null +++ b/module/Core/src/Model/Visitor.php @@ -0,0 +1,47 @@ +userAgent = $userAgent; + $this->referer = $referer; + $this->remoteAddress = $remoteAddress; + } + + public static function emptyInstance(): self + { + return new self('', '', null); + } + + public function getUserAgent(): string + { + return $this->userAgent; + } + + public function getReferer(): string + { + return $this->referer; + } + + public function getRemoteAddress(): ?string + { + return $this->remoteAddress; + } +} diff --git a/module/Core/src/Service/VisitsTracker.php b/module/Core/src/Service/VisitsTracker.php index bc2a4305..1f40b245 100644 --- a/module/Core/src/Service/VisitsTracker.php +++ b/module/Core/src/Service/VisitsTracker.php @@ -4,11 +4,11 @@ declare(strict_types=1); namespace Shlinkio\Shlink\Core\Service; use Doctrine\ORM; -use Psr\Http\Message\ServerRequestInterface; use Shlinkio\Shlink\Common\Util\DateRange; use Shlinkio\Shlink\Core\Entity\ShortUrl; use Shlinkio\Shlink\Core\Entity\Visit; use Shlinkio\Shlink\Core\Exception\InvalidArgumentException; +use Shlinkio\Shlink\Core\Model\Visitor; use Shlinkio\Shlink\Core\Repository\VisitRepository; class VisitsTracker implements VisitsTrackerInterface @@ -24,14 +24,9 @@ class VisitsTracker implements VisitsTrackerInterface } /** - * Tracks a new visit to provided short code, using an array of data to look up information - * - * @param string $shortCode - * @param ServerRequestInterface $request - * @throws ORM\ORMInvalidArgumentException - * @throws ORM\OptimisticLockException + * Tracks a new visit to provided short code from provided visitor */ - public function track($shortCode, ServerRequestInterface $request): void + public function track(string $shortCode, Visitor $visitor): void { /** @var ShortUrl $shortUrl */ $shortUrl = $this->em->getRepository(ShortUrl::class)->findOneBy([ @@ -40,9 +35,9 @@ class VisitsTracker implements VisitsTrackerInterface $visit = new Visit(); $visit->setShortUrl($shortUrl) - ->setUserAgent($request->getHeaderLine('User-Agent')) - ->setReferer($request->getHeaderLine('Referer')) - ->setRemoteAddr($this->findOutRemoteAddr($request)); + ->setUserAgent($visitor->getUserAgent()) + ->setReferer($visitor->getReferer()) + ->setRemoteAddr($visitor->getRemoteAddress()); /** @var ORM\EntityManager $em */ $em = $this->em; @@ -50,21 +45,6 @@ class VisitsTracker implements VisitsTrackerInterface $em->flush($visit); } - /** - * @param ServerRequestInterface $request - */ - private function findOutRemoteAddr(ServerRequestInterface $request): ?string - { - $forwardedFor = $request->getHeaderLine('X-Forwarded-For'); - if (empty($forwardedFor)) { - $serverParams = $request->getServerParams(); - return $serverParams['REMOTE_ADDR'] ?? null; - } - - $ips = \explode(',', $forwardedFor); - return $ips[0] ?? null; - } - /** * Returns the visits on certain short code * diff --git a/module/Core/src/Service/VisitsTrackerInterface.php b/module/Core/src/Service/VisitsTrackerInterface.php index 75bc8d6f..79676864 100644 --- a/module/Core/src/Service/VisitsTrackerInterface.php +++ b/module/Core/src/Service/VisitsTrackerInterface.php @@ -3,20 +3,17 @@ declare(strict_types=1); namespace Shlinkio\Shlink\Core\Service; -use Psr\Http\Message\ServerRequestInterface; use Shlinkio\Shlink\Common\Util\DateRange; use Shlinkio\Shlink\Core\Entity\Visit; use Shlinkio\Shlink\Core\Exception\InvalidArgumentException; +use Shlinkio\Shlink\Core\Model\Visitor; interface VisitsTrackerInterface { /** - * Tracks a new visit to provided short code, using an array of data to look up information - * - * @param string $shortCode - * @param ServerRequestInterface $request + * Tracks a new visit to provided short code from provided visitor */ - public function track($shortCode, ServerRequestInterface $request): void; + public function track(string $shortCode, Visitor $visitor): void; /** * Returns the visits on certain short code diff --git a/module/Core/test/Service/VisitsTrackerTest.php b/module/Core/test/Service/VisitsTrackerTest.php index f9703e3b..154d6367 100644 --- a/module/Core/test/Service/VisitsTrackerTest.php +++ b/module/Core/test/Service/VisitsTrackerTest.php @@ -10,6 +10,7 @@ use Prophecy\Argument; use Prophecy\Prophecy\ObjectProphecy; use Shlinkio\Shlink\Core\Entity\ShortUrl; use Shlinkio\Shlink\Core\Entity\Visit; +use Shlinkio\Shlink\Core\Model\Visitor; use Shlinkio\Shlink\Core\Repository\VisitRepository; use Shlinkio\Shlink\Core\Service\VisitsTracker; use Zend\Diactoros\ServerRequestFactory; @@ -44,13 +45,13 @@ class VisitsTrackerTest extends TestCase $this->em->persist(Argument::any())->shouldBeCalledTimes(1); $this->em->flush(Argument::type(Visit::class))->shouldBeCalledTimes(1); - $this->visitsTracker->track($shortCode, ServerRequestFactory::fromGlobals()); + $this->visitsTracker->track($shortCode, Visitor::emptyInstance()); } /** * @test */ - public function trackUsesForwardedForHeaderIfPresent() + public function trackedIpAddressGetsObfuscated() { $shortCode = '123ABC'; $test = $this; @@ -65,9 +66,7 @@ class VisitsTrackerTest extends TestCase })->shouldBeCalledTimes(1); $this->em->flush(Argument::type(Visit::class))->shouldBeCalledTimes(1); - $this->visitsTracker->track($shortCode, ServerRequestFactory::fromGlobals( - ['REMOTE_ADDR' => '1.2.3.4'] - )->withHeader('X-Forwarded-For', '4.3.2.1,99.99.99.99')); + $this->visitsTracker->track($shortCode, new Visitor('', '', '4.3.2.1')); } /** From 44f0011445c6f9c66cf08739fbcc3902df1ff13f Mon Sep 17 00:00:00 2001 From: Alejandro Celaya Date: Thu, 18 Oct 2018 20:24:25 +0200 Subject: [PATCH 7/9] Moved logic to create a visitor from a request to the visitor itself --- .../src/Middleware/IpAddressMiddlewareFactory.php | 5 ++--- .../Middleware/IpAddressMiddlewareFactoryTest.php | 3 ++- module/Core/src/Action/AbstractTrackingAction.php | 7 +------ module/Core/src/Model/Visitor.php | 13 +++++++++++++ module/Core/test/Service/VisitsTrackerTest.php | 1 - 5 files changed, 18 insertions(+), 11 deletions(-) diff --git a/module/Common/src/Middleware/IpAddressMiddlewareFactory.php b/module/Common/src/Middleware/IpAddressMiddlewareFactory.php index a8689e55..b9cd3879 100644 --- a/module/Common/src/Middleware/IpAddressMiddlewareFactory.php +++ b/module/Common/src/Middleware/IpAddressMiddlewareFactory.php @@ -5,14 +5,13 @@ namespace Shlinkio\Shlink\Common\Middleware; use Interop\Container\ContainerInterface; use RKA\Middleware\IpAddress; +use Shlinkio\Shlink\Core\Model\Visitor; use Zend\ServiceManager\Exception\ServiceNotCreatedException; use Zend\ServiceManager\Exception\ServiceNotFoundException; use Zend\ServiceManager\Factory\FactoryInterface; class IpAddressMiddlewareFactory implements FactoryInterface { - public const REMOTE_ADDRESS = 'remote_address'; - /** * Create an object * @@ -24,6 +23,6 @@ class IpAddressMiddlewareFactory implements FactoryInterface */ public function __invoke(ContainerInterface $container, $requestedName, array $options = null): IpAddress { - return new IpAddress(true, [], self::REMOTE_ADDRESS); + return new IpAddress(true, [], Visitor::REMOTE_ADDRESS_ATTR); } } diff --git a/module/Common/test/Middleware/IpAddressMiddlewareFactoryTest.php b/module/Common/test/Middleware/IpAddressMiddlewareFactoryTest.php index 2087e5c3..4c744b7c 100644 --- a/module/Common/test/Middleware/IpAddressMiddlewareFactoryTest.php +++ b/module/Common/test/Middleware/IpAddressMiddlewareFactoryTest.php @@ -6,6 +6,7 @@ namespace ShlinkioTest\Shlink\Common\Middleware; use PHPUnit\Framework\TestCase; use ReflectionObject; use Shlinkio\Shlink\Common\Middleware\IpAddressMiddlewareFactory; +use Shlinkio\Shlink\Core\Model\Visitor; use Zend\ServiceManager\ServiceManager; class IpAddressMiddlewareFactoryTest extends TestCase @@ -34,6 +35,6 @@ class IpAddressMiddlewareFactoryTest extends TestCase $this->assertTrue($checkProxyHeaders->getValue($instance)); $this->assertEquals([], $trustedProxies->getValue($instance)); - $this->assertEquals(IpAddressMiddlewareFactory::REMOTE_ADDRESS, $attributeName->getValue($instance)); + $this->assertEquals(Visitor::REMOTE_ADDRESS_ATTR, $attributeName->getValue($instance)); } } diff --git a/module/Core/src/Action/AbstractTrackingAction.php b/module/Core/src/Action/AbstractTrackingAction.php index 778396d9..0d118565 100644 --- a/module/Core/src/Action/AbstractTrackingAction.php +++ b/module/Core/src/Action/AbstractTrackingAction.php @@ -9,7 +9,6 @@ use Psr\Http\Server\MiddlewareInterface; use Psr\Http\Server\RequestHandlerInterface; use Psr\Log\LoggerInterface; use Psr\Log\NullLogger; -use Shlinkio\Shlink\Common\Middleware\IpAddressMiddlewareFactory; use Shlinkio\Shlink\Core\Action\Util\ErrorResponseBuilderTrait; use Shlinkio\Shlink\Core\Exception\EntityDoesNotExistException; use Shlinkio\Shlink\Core\Exception\InvalidShortCodeException; @@ -71,11 +70,7 @@ abstract class AbstractTrackingAction implements MiddlewareInterface // Track visit to this short code if ($disableTrackParam === null || ! \array_key_exists($disableTrackParam, $query)) { - $this->visitTracker->track($shortCode, new Visitor( - $request->getHeaderLine('User-Agent'), - $request->getHeaderLine('Referer'), - $request->getAttribute(IpAddressMiddlewareFactory::REMOTE_ADDRESS) - )); + $this->visitTracker->track($shortCode, Visitor::fromRequest($request)); } return $this->createResp($url->getLongUrl()); diff --git a/module/Core/src/Model/Visitor.php b/module/Core/src/Model/Visitor.php index c6a828f0..82cb3cae 100644 --- a/module/Core/src/Model/Visitor.php +++ b/module/Core/src/Model/Visitor.php @@ -3,8 +3,12 @@ declare(strict_types=1); namespace Shlinkio\Shlink\Core\Model; +use Psr\Http\Message\ServerRequestInterface; + final class Visitor { + public const REMOTE_ADDRESS_ATTR = 'remote_address'; + /** * @var string */ @@ -25,6 +29,15 @@ final class Visitor $this->remoteAddress = $remoteAddress; } + public static function fromRequest(ServerRequestInterface $request): self + { + return new self( + $request->getHeaderLine('User-Agent'), + $request->getHeaderLine('Referer'), + $request->getAttribute(self::REMOTE_ADDRESS_ATTR) + ); + } + public static function emptyInstance(): self { return new self('', '', null); diff --git a/module/Core/test/Service/VisitsTrackerTest.php b/module/Core/test/Service/VisitsTrackerTest.php index 154d6367..fd794245 100644 --- a/module/Core/test/Service/VisitsTrackerTest.php +++ b/module/Core/test/Service/VisitsTrackerTest.php @@ -13,7 +13,6 @@ use Shlinkio\Shlink\Core\Entity\Visit; use Shlinkio\Shlink\Core\Model\Visitor; use Shlinkio\Shlink\Core\Repository\VisitRepository; use Shlinkio\Shlink\Core\Service\VisitsTracker; -use Zend\Diactoros\ServerRequestFactory; class VisitsTrackerTest extends TestCase { From 9f4c2ac8d7850a653a8ff01face529fc39f5be43 Mon Sep 17 00:00:00 2001 From: Alejandro Celaya Date: Thu, 18 Oct 2018 20:26:44 +0200 Subject: [PATCH 8/9] Inlined instructions to enable apcu and memcached in travis --- .travis.yml | 4 ++-- data/infra/travis-php/apcu.ini | 1 - data/infra/travis-php/memcached.ini | 1 - 3 files changed, 2 insertions(+), 4 deletions(-) delete mode 100644 data/infra/travis-php/apcu.ini delete mode 100644 data/infra/travis-php/memcached.ini diff --git a/.travis.yml b/.travis.yml index 76428e67..b09b5035 100644 --- a/.travis.yml +++ b/.travis.yml @@ -16,8 +16,8 @@ matrix: - php: 7.3 before_install: - - phpenv config-add data/infra/travis-php/memcached.ini - - phpenv config-add data/infra/travis-php/apcu.ini + - echo 'extension = memcached.so' >> ~/.phpenv/versions/$(phpenv version-name)/etc/php.ini + - echo 'extension = apcu.so' >> ~/.phpenv/versions/$(phpenv version-name)/etc/php.ini install: - composer self-update diff --git a/data/infra/travis-php/apcu.ini b/data/infra/travis-php/apcu.ini deleted file mode 100644 index 4a2dd79a..00000000 --- a/data/infra/travis-php/apcu.ini +++ /dev/null @@ -1 +0,0 @@ -extension="apcu.so" diff --git a/data/infra/travis-php/memcached.ini b/data/infra/travis-php/memcached.ini deleted file mode 100644 index 6b9b24f3..00000000 --- a/data/infra/travis-php/memcached.ini +++ /dev/null @@ -1 +0,0 @@ -extension="memcached.so" From e66a724d2b742a987f9f86cb81eaa0ab21be2588 Mon Sep 17 00:00:00 2001 From: Alejandro Celaya Date: Thu, 18 Oct 2018 20:34:55 +0200 Subject: [PATCH 9/9] Added fix on IP addresses discovery to changelog --- CHANGELOG.md | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index b7f237a8..9e48c600 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -25,7 +25,9 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/), #### Fixed -* *Nothing* +* [#237](https://github.com/shlinkio/shlink/issues/233) Solved errors when trying to geo-locate `null` IP addresses. + + Also improved how visitor IP addresses are discovered, thanks to [akrabat/ip-address-middleware](https://github.com/akrabat/ip-address-middleware) package. ## 1.13.1 - 2018-10-16