WPK-8, #339 - tweak IP rate limit checks

* use a more Symfony-standard way to determine forwarded IPs
* move request table check/update logic to a repository helper
* use QB + ORM there as appropriate for cleaner code & fewer DB vendor hacks

src/Controller/MainController.php

@@ -12,7 +12,6 @@
 use Outlandish\Wpackagist\Service;
 use Pagerfanta\Doctrine\ORM\QueryAdapter;
 use Pagerfanta\Pagerfanta;
-use PDO;
 use Psr\Log\LoggerInterface;
 use Symfony\Bundle\FrameworkBundle\Controller\AbstractController;
 use Symfony\Component\Form\Extension\Core\Type\ChoiceType;
@@ -27,6 +26,7 @@
 use Symfony\Component\HttpFoundation\Response;
 use Symfony\Component\HttpKernel\Exception\NotFoundHttpException;
 use Symfony\Component\Routing\Annotation\Route;
+use Outlandish\Wpackagist\Entity\Request as RequestLog;
 use Outlandish\Wpackagist\Storage;
 
 class MainController extends AbstractController
@@ -204,20 +204,13 @@ public function update(
             return new Response('Not Found',404);
         }
 
-        $safeName = $package->getName();
-
-        if (isset($_SERVER['HTTP_X_FORWARDED_FOR']) && $_SERVER['HTTP_X_FORWARDED_FOR']) {
-            $splitIp = explode(',', $_SERVER['HTTP_X_FORWARDED_FOR']);
-            $userIp  = trim($splitIp[0]);
-        } else {
-            $userIp = $_SERVER['REMOTE_ADDR'];
-        }
-
-        $count = $this->getRequestCountByIp($userIp, $connection);
-        if ($count > 5) {
+        $requestCount = $entityManager->getRepository(RequestLog::class)
+            ->getRequestCountByIp($request->getClientIp());
+        if ($requestCount > 5) {
             return new Response('Too many requests. Try again in an hour.', 403);
         }
 
+        $safeName = $package->getName();
         $package = $updateService->updateOne($logger, $safeName);
         if ($package && !empty($package->getVersions()) && $package->isActive()) {
             // update just the package
@@ -260,55 +253,4 @@ private function getForm(): FormInterface
 
         return $this->form;
     }
-
-    /**
-     * @param string $ip
-     * @param Connection $db
-     * @return int The number of requests within the past 24 hours
-     * @throws \Doctrine\DBAL\DBALException
-     * @todo move to a Repository helper?
-     */
-    private function getRequestCountByIp(string $ip, Connection $db): int
-    {
-        $query = $db->prepare(
-            "SELECT * FROM requests WHERE ip_address = :ip AND last_request > :cutoff"
-        );
-        $query->execute([
-            'cutoff' => (new \DateTime())->sub(new \DateInterval('PT1H'))->format($db->getDatabasePlatform()->getDateTimeFormatString()),
-            'ip' => $ip,
-        ]);
-
-        $requestHistory = $query->fetch(PDO::FETCH_ASSOC);
-        if (!$requestHistory) {
-            $this->resetRequestCount($ip, $db);
-            return 1;
-        }
-
-        $update = $db->prepare(
-            'UPDATE requests SET request_count = request_count + 1, last_request = CURRENT_TIMESTAMP WHERE ip_address = :ip'
-        );
-
-        $update->execute([ $ip ]);
-        return $requestHistory['request_count'] + 1;
-    }
-
-    /**
-     * Add an entry to the requests table for the provided IP address.
-     * Has the side effect of removing all expired entries.
-     *
-     * @param string $ip
-     * @param Connection $db
-     * @throws \Doctrine\DBAL\DBALException
-     * @todo move to a Repository helper?
-     */
-    private function resetRequestCount(string $ip, Connection $db)
-    {
-        $prune = $db->prepare('DELETE FROM requests WHERE last_request < :cutoff');
-        $prune->bindValue('cutoff', (new \DateTime())->sub(new \DateInterval('PT1H'))->format($db->getDatabasePlatform()->getDateTimeFormatString()));
-        $prune->execute();
-        $insert = $db->prepare(
-            'INSERT INTO requests (ip_address, last_request, request_count) VALUES (:ip_address, CURRENT_TIMESTAMP, 1)'
-        );
-        $insert->execute([ $ip ]);
-    }
 }

src/Entity/Request.php

@@ -6,7 +6,7 @@
 use Doctrine\ORM\Mapping as ORM;
 
 /**
- * @ORM\Entity()
+ * @ORM\Entity(repositoryClass=RequestRepository::class)
  * @ORM\Table(name="requests")
  */
 class Request
@@ -35,5 +35,21 @@ class Request
      * @ORM\Column(type="integer")
      * @var int
      */
-    protected $requestCount;
+    protected $requestCount = 0;
+
+    public function addRequest(): void
+    {
+        $this->requestCount++;
+        $this->lastRequest = new \DateTime();
+    }
+
+    public function getRequestCount(): int
+    {
+        return $this->requestCount;
+    }
+
+    public function setIpAddress(string $ipAddress): void
+    {
+        $this->ipAddress = $ipAddress;
+    }
 }

src/Entity/RequestRepository.php

@@ -0,0 +1,75 @@
+<?php
+
+namespace Outlandish\Wpackagist\Entity;
+
+use Doctrine\ORM\EntityRepository;
+use Doctrine\ORM\QueryBuilder;
+
+class RequestRepository extends EntityRepository
+{
+    /**
+     * Get a count of sensitive requests for an IP, incrementing the counter as a side effect.
+     *
+     * @param string $ip
+     * @return int The number of requests within the past 24 hours
+     * @throws \Doctrine\DBAL\DBALException
+     */
+    public function getRequestCountByIp(string $ip): int
+    {
+        $em = $this->getEntityManager();
+        $qb = new QueryBuilder($em);
+
+        $oneHourAgo = (new \DateTime())->sub(new \DateInterval('PT1H'));
+
+        $qb->select('r')
+            ->from(Request::class, 'r')
+            ->where('r.ipAddress = :ip')
+            ->andWhere('r.lastRequest >= :cutoff')
+            ->setParameter('ip', $ip)
+            ->setParameter('cutoff', $oneHourAgo);
+        $requestHistory = $qb->getQuery()->getResult();
+
+        if (empty($requestHistory)) {
+            $this->resetRequestCount($ip, $oneHourAgo);
+            return 1;
+        }
+
+        /** @var Request $requestItem */
+        $requestItem = $requestHistory[0];
+        $requestItem->addRequest();
+        $em->persist($requestItem);
+
+        return $requestItem->getRequestCount();
+    }
+
+    /**
+     * Add an entry to the requests table for the provided IP address.
+     * Has the side effect of removing all expired entries.
+     *
+     * @param string $ip
+     * @param \DateTime $cutoff
+     */
+    private function resetRequestCount(string $ip, \DateTime $cutoff)
+    {
+        // Prune any old records.
+        $em = $this->getEntityManager();
+        $qb = new QueryBuilder($em);
+        $qb->delete(Request::class, 'r')
+            ->where('r.ipAddress = :ip')
+            ->andWhere('r.lastRequest < :cutoff')
+            ->setParameter('ip', $ip)
+            ->setParameter('cutoff', $cutoff)
+            ->getQuery()
+            ->execute();
+
+        // Ensure the old record is deleted at DB level before the insert coming up, so we don't
+        // fall foul of the IP uniqueness constraint.
+        $em->flush();
+
+        // Add a new Request record and set it up with `requestCount` 1 and `lastRequest` now.
+        $requestItem = new Request();
+        $requestItem->setIpAddress($ip);
+        $requestItem->addRequest();
+        $em->persist($requestItem);
+    }
+}

web/index.php

@@ -14,6 +14,15 @@
 
 if ($trustedProxies = $_SERVER['TRUSTED_PROXIES'] ?? false) {
     Request::setTrustedProxies(explode(',', $trustedProxies), Request::HEADER_X_FORWARDED_ALL ^ Request::HEADER_X_FORWARDED_HOST);
+} else {
+    /**
+     * If no env override, get the correct request context behind an AWS load balancer.
+     * @link https://symfony.com/doc/5.2/deployment/proxies.html
+     */
+    Request::setTrustedProxies(
+        ['127.0.0.1', 'REMOTE_ADDR'], // REMOTE_ADDR string replaced at runtime.
+        Request::HEADER_X_FORWARDED_AWS_ELB
+    );
 }
 
 if ($trustedHosts = $_SERVER['TRUSTED_HOSTS'] ?? false) {