Merge pull request #238 from acelaya/feature/fix-ip-address

Feature/fix ip address

.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

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

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",

data/infra/travis-php/apcu.ini

@@ -1 +0,0 @@
-extension="apcu.so"

data/infra/travis-php/memcached.ini

@@ -1 +0,0 @@
-extension="memcached.so"

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('<comment>%s</comment>', $this->translator->translate('Ignored visit with no IP address')),
+                    OutputInterface::VERBOSITY_VERBOSE
+                );
+                continue;
+            }
+
             $ipAddr = $visit->getRemoteAddr();
             $io->write(sprintf('%s <info>%s</info>', $this->translator->translate('Processing IP'), $ipAddr));
             if ($ipAddr === IpAddress::LOCALHOST) {

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));
     }
 }

module/Common/config/dependencies.config.php

@@ -1,16 +1,14 @@
 <?php
 declare(strict_types=1);
 
+namespace Shlinkio\Shlink\Common;
+
 use Doctrine\Common\Cache\Cache;
 use Doctrine\ORM\EntityManager;
+use GuzzleHttp\Client as GuzzleClient;
 use Monolog\Logger;
 use Psr\Log\LoggerInterface;
-use Shlinkio\Shlink\Common\Factory;
-use Shlinkio\Shlink\Common\Image;
-use Shlinkio\Shlink\Common\Image\ImageBuilder;
-use Shlinkio\Shlink\Common\Middleware\LocaleMiddleware;
-use Shlinkio\Shlink\Common\Service;
-use Shlinkio\Shlink\Common\Template\Extension\TranslatorExtension;
+use RKA\Middleware\IpAddress;
 use Symfony\Component\Filesystem\Filesystem;
 use Zend\I18n\Translator\Translator;
 use Zend\ServiceManager\AbstractFactory\ConfigAbstractFactory;
@@ -21,14 +19,16 @@ return [
     'dependencies' => [
         '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',
         ],

module/Common/src/Middleware/IpAddressMiddlewareFactory.php

@@ -0,0 +1,28 @@
+<?php
+declare(strict_types=1);
+
+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
+{
+    /**
+     * Create an object
+     *
+     * @param  ContainerInterface $container
+     * @param  string $requestedName
+     * @param  null|array $options
+     * @throws ServiceNotFoundException if unable to resolve the service.
+     * @throws ServiceNotCreatedException if an exception is raised when creating a service.
+     */
+    public function __invoke(ContainerInterface $container, $requestedName, array $options = null): IpAddress
+    {
+        return new IpAddress(true, [], Visitor::REMOTE_ADDRESS_ATTR);
+    }
+}

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,

module/Common/test/Middleware/IpAddressMiddlewareFactoryTest.php

@@ -0,0 +1,40 @@
+<?php
+declare(strict_types=1);
+
+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
+{
+    private $factory;
+
+    public function setUp()
+    {
+        $this->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));
+    }
+}

module/Core/config/routes.config.php

@@ -1,6 +1,7 @@
 <?php
 declare(strict_types=1);
 
+use RKA\Middleware\IpAddress;
 use Shlinkio\Shlink\Core\Action;
 use Shlinkio\Shlink\Core\Middleware;
 
@@ -10,13 +11,19 @@ return [
         [
             'name' => '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'],
         ],
         [

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());

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

module/Core/src/Model/Visitor.php

@@ -0,0 +1,60 @@
+<?php
+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
+     */
+    private $userAgent;
+    /**
+     * @var string
+     */
+    private $referer;
+    /**
+     * @var string|null
+     */
+    private $remoteAddress;
+
+    public function __construct(string $userAgent, string $referer, ?string $remoteAddress)
+    {
+        $this->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;
+    }
+}

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
      *

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

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'));
     }
 
     /**