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