diff --git a/.travis.yml b/.travis.yml index e290684f..b09b5035 100644 --- a/.travis.yml +++ b/.travis.yml @@ -9,10 +9,15 @@ branches: php: - 7.1 - 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 + - 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/CHANGELOG.md b/CHANGELOG.md index 486dddac..9e48c600 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,5 +1,35 @@ # 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 PHP 7.3 to build matrix allowing its failure. + +#### Changed + +* *Nothing* + +#### Deprecated + +* *Nothing* + +#### Removed + +* *Nothing* + +#### Fixed + +* [#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 #### Added 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/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" 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/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..b9cd3879 --- /dev/null +++ b/module/Common/src/Middleware/IpAddressMiddlewareFactory.php @@ -0,0 +1,28 @@ +firstOctet, $this->secondOctet, $this->thirdOctet, diff --git a/module/Common/test/Middleware/IpAddressMiddlewareFactoryTest.php b/module/Common/test/Middleware/IpAddressMiddlewareFactoryTest.php new file mode 100644 index 00000000..4c744b7c --- /dev/null +++ b/module/Common/test/Middleware/IpAddressMiddlewareFactoryTest.php @@ -0,0 +1,40 @@ +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); + $attributeName = $ref->getProperty('attributeName'); + $attributeName->setAccessible(true); + + $this->assertTrue($checkProxyHeaders->getValue($instance)); + $this->assertEquals([], $trustedProxies->getValue($instance)); + $this->assertEquals(Visitor::REMOTE_ADDRESS_ATTR, $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..0d118565 100644 --- a/module/Core/src/Action/AbstractTrackingAction.php +++ b/module/Core/src/Action/AbstractTrackingAction.php @@ -12,6 +12,7 @@ use Psr\Log\NullLogger; 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 +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, $request); + $this->visitTracker->track($shortCode, Visitor::fromRequest($request)); } return $this->createResp($url->getLongUrl()); 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 diff --git a/module/Core/src/Model/Visitor.php b/module/Core/src/Model/Visitor.php new file mode 100644 index 00000000..82cb3cae --- /dev/null +++ b/module/Core/src/Model/Visitor.php @@ -0,0 +1,60 @@ +userAgent = $userAgent; + $this->referer = $referer; + $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); + } + + 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..fd794245 100644 --- a/module/Core/test/Service/VisitsTrackerTest.php +++ b/module/Core/test/Service/VisitsTrackerTest.php @@ -10,9 +10,9 @@ 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; class VisitsTrackerTest extends TestCase { @@ -44,13 +44,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 +65,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')); } /**